Extract default conversation agent
Proposed change
Extract default conversation agent from the conversation integration into a separate component, improving the architecture and increasing modularity of code.
With this change conversation integration will stay responsible for defining the conversation entity and functionality related to that (chat log, etc.), whereas the new integration will provide the default conversation agent.
I tried to limit the diff here, so the split is not as clean as I would like yet. This will be improved in follow-up PRs.
I named the new component assist_conversation, per analogiam to openai_conversation, google_generative_ai_conversation, etc.; and also in line with assist_pipeline and assist_satellite.
TODO:
- write docs
Type of change
- [ ] Dependency upgrade
- [ ] Bugfix (non-breaking change which fixes an issue)
- [x] New integration (thank you!)
- [ ] New feature (which adds functionality to an existing integration)
- [ ] Deprecation (breaking change to happen in the future)
- [ ] Breaking change (fix/feature causing existing functionality to break)
- [ ] Code quality improvements to existing code or addition of tests
Additional information
- This PR fixes or closes issue: fixes #
- This PR is related to issue:
- Link to documentation pull request:
- Link to developer documentation pull request:
- Link to frontend pull request:
Checklist
- [x] The code change is tested and works locally.
- [x] Local tests pass. Your PR cannot be merged unless tests pass
- [x] There is no commented out code in this PR.
- [x] I have followed the development checklist
- [x] I have followed the perfect PR recommendations
- [x] The code has been formatted using Ruff (
ruff format homeassistant tests) - [x] Tests have been added to verify that the new code works.
If user exposed functionality or configuration variables are added/changed:
- [ ] Documentation added/updated for www.home-assistant.io
If the code communicates with devices, web services, or third-party tools:
- [x] The manifest file has all fields filled out correctly.
Updated and included derived files by running:python3 -m script.hassfest. - [x] New or updated dependencies have been added to
requirements_all.txt.
Updated by runningpython3 -m script.gen_requirements_all. - [ ] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
To help with the load of incoming pull requests:
- [x] I have reviewed two other open pull requests in this repository.
Hey there @balloob, @synesthesiam, mind taking a look at this pull request as it has been labeled with an integration (assist_pipeline) you are listed as a code owner for? Thanks!
Code owner commands
Code owners of assist_pipeline can trigger bot actions by commenting:
-
@home-assistant closeCloses the pull request. -
@home-assistant rename Awesome new titleRenames the pull request. -
@home-assistant reopenReopen the pull request. -
@home-assistant unassign assist_pipelineRemoves the current integration label and assignees on the pull request, add the integration domain after the command. -
@home-assistant add-label needs-more-informationAdd a label (needs-more-information, problem in dependency, problem in custom component) to the pull request. -
@home-assistant remove-label needs-more-informationRemove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.
Hey there @home-assistant/core, @synesthesiam, mind taking a look at this pull request as it has been labeled with an integration (conversation) you are listed as a code owner for? Thanks!
Code owner commands
Code owners of conversation can trigger bot actions by commenting:
-
@home-assistant closeCloses the pull request. -
@home-assistant rename Awesome new titleRenames the pull request. -
@home-assistant reopenReopen the pull request. -
@home-assistant unassign conversationRemoves the current integration label and assignees on the pull request, add the integration domain after the command. -
@home-assistant add-label needs-more-informationAdd a label (needs-more-information, problem in dependency, problem in custom component) to the pull request. -
@home-assistant remove-label needs-more-informationRemove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.
When the PR is accepted, I will add docs (and brand).
Other than that the PR is ready to review. I think in the end the split came out quite clean, honestly more than I expected at the beginning. With this merged, the further work on improving the default agent will be much easier.
Can you talk a bit more about the end goal here? After looking into it more, I'm wondering if we may achieve similar goals with Wyoming and some more config options.
@synesthesiam
Absolutely! Here are the reasons why I think this change is a good idea:
-
For a start, I think that functionality is complex and extensive and deserves to be in its own component.
-
Having it directly in a
conversationintegration is particularly problematic.conversationis a base entity platform and those are quite special in HA. From my experience with work on HA initialisation and related issues, I consider it a good practice to have those be as lean as possible. Especially having complex dependencies on them should ideally be avoided. -
conversationshould be focused on providing the base entity implementation and related functionality (chatlog, etc.), which is already vast by itself and leave the also major task of interpreting and understanding user's intent in a text to a separate component. -
Since
conversationis a base platform, it is being imported all over the place, which brings with it an unfortunate limitation of being hard to replace as a custom component. With most integrations it is possible to just take a copy of them, make some interesting changes and simply drop it in as a custom component on a working HA instance. That way is not really possible for a base platform. -
Having it all in a single component, makes it all very strongly coupled. This in turn leads to sub-optimal architecture and engineering decisions. It makes the code, which is already quite complex here, even harder to understand and reason about. In fact, just when doing this PR, I've already caught some problems related to that.
-
Finally it also makes it harder to move into a possible future where users can replace their default agent with a more sophisticated one. I think that is what you are referring to with "similar goals with Wyoming and some more config options". Having it less coupled makes it easier to design for that, and also prevents other components from making direct assumptions, that can in general be false. I would consider this PR to be a prerequisite of that path, but I believe it is also worth doing on its own.
So to sum up the above succinctly: making code more modular actually decreases its complexity and simply makes it better in the long run.
@arturpragacz After more thought and discussions, we don't plan to move forward with this. While I agree with your points about code organization and responsibility, the added maintenance burden of splitting doesn't seem worth it to me at this point in time.
I appreciate all of the time and effort you put into this PR and proposal. Let's think about ways that we can either make the default agent more extensible (without needing to be replaced), or how sentence trigger handling may be done so that replacing the default agent with an external Wyoming agent still handles triggers.