KG-505. Move Debugger feature to the agents-core #1032
Conversation
sdubov
added
refactoring
sdubov
force-pushed
the
sdubov/KG-505
branch
8 times, most recently
from
aeee620 to
b2685ce
Compare
Qodana for JVM17426 new problems were found
@@ Code coverage @@
+ 70% total lines covered
15997 lines analyzed, 11318 lines covered
# Calculated according to the filters of your coverage tool☁️ View the detailed Qodana report Contact Qodana teamContact us at [email protected]
|
There was a problem hiding this comment.
While I understand the reason why this change was made - we want to always ensure that Debugger is installed - I have some concerns regarding the implementation. Specifically, moving feature implementation to the core, as it kinda obscures the border between core and non-core.
I have an alternative proposal. What if we go another way, and move some stuff out of the core, to make it lighter and leave truly "core" functionality in there. Namely, I'm suggesting to move GraphAIAgent and FunctionalAIAgent with AgentService to a separate module(s). It would give us a number of benefits and would work as follows:
- Keeping the core lean without bringing specific implementation details. Only truly "core" functionality is left in core, and concrete implementations leave in dedicated modules.
- You get to keep debugger with all its logic in a separate module, as before, making its development more isolated and easier. Faster compilation and no unnecesary clashes and conflicts with changes in core.
- To still ensure that debugger is always installed in the agents we provide, we just add an explicit dependency in module with concrete agent implementations that we moved out of the core before on the debugger feature, and install it explicitly, like any other feature. We can safely do this now, since it's a separate module and there won't be circular dependencies.
IMHO such solution would look cleaner and ensure better structure. Please let me know what you think, and let's maybe discuss it in more details.
| /** | ||
| * A set containing the keys of all registered features. | ||
| */ | ||
| public val features: Set<AIAgentStorageKey<*>> |
There was a problem hiding this comment.
This is basically a duplicate of a registeredFeatures, so it is doubtful that a separate property is needed. And it is used in tests only, so shouldn't it be removed?
There was a problem hiding this comment.
True, this was added for the test purpose, but there is a reason for that. registeredFeatures is protected and store a feature and feature config. If I make this public, the config will be exposed to public as well. It means that you can modify the config outside of the install(MyFeature) { ... } lambda which I did not want to break the current design.
It might be that we want to expose this, and, I can imagine, that this can be helpful in some cases. For example, when you want to spread the control for the feature config. I would propose to discuss this topic separately.
- Move Debugger feature from agents-features module into agents-core module to get access to the feature from the core agents flow and be able to install a feature through an external agent configuration.
- Add a list of system features (Debugger) that can be installed through agent configuration parameters; - Add agent logic to read a system property and set up Debugger feature with a provided config; - Add tests to check updates in an agent pipeline logic.
sdubov
force-pushed
the
sdubov/KG-505
branch
2 times, most recently
from
9fbbd99 to
465ef2b
Compare
KG-505. Move Debugger feature to the agents-core
Motivation and Context
Debugger feature is essential for a debugging capabilities. It would be necessary to have it in agents-core to enable the functionality upon request.
This is also allows to install debugger feature upon request by a plugins, e.g. "AI Debugger" that can set the Debugger feature via VM options instead of requesting this from a user.
Breaking Changes
Yes
Type of the changes
Checklist
developas the base branchAdditional steps for pull requests adding a new feature