-
Notifications
You must be signed in to change notification settings - Fork 35
chore: include version configuration for agent tool #2324
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2324 +/- ##
==========================================
- Coverage 90.78% 90.78% -0.01%
==========================================
Files 170 170
Lines 25662 25665 +3
==========================================
+ Hits 23297 23299 +2
- Misses 2365 2366 +1
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have yet not tested it, but see comment about string literal
Args: | ||
data_models (Sequence[DataModelInfo] | None): The data models and views to query. | ||
instance_spaces (InstanceSpaces | None): The instance spaces to query. | ||
version (str | None): The version of the query generation strategy to use. A higher number does not necessarily mean a better query. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would maybe like that we have string literals v1
or v2
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andeplane I considered that, but it gives a tight coupling that needs to be updated again for example if there comes a "v3". On the other hand, string literals is better guidance to the user. How frequently do we expect new versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We must not break old SDK versions if the API suddenly returns v3
etc, so as long as we in docstrings cover valid values (v1
and v2
as of today), and no default value since old agents before we added this field may have no value set (effectively v1
behaviour), we should not overwrite with a v2
default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice
Co-authored-by: Anders Albert <[email protected]>
Risk review: One outstanding comment from Erlend to be addressed, then ready for unicorn 😄 |
Co-authored-by: Erlend vollset <[email protected]>
@ronpal sorry for not getting back to you sooner, please ping me on slack for follow-up on risk review if it takes more than 1 hour 😜 Re. @erlendvollset comment above:
When I take a look at the class: Also, from your screenshot, it seems like If all of this is true, I think the correct solution is to add back default arg |
Description
Adding version to the
QueryKnowledgeGraphAgentToolConfiguration
to be on par with the API. Chose not to go with enum since string is more future-proof.Please describe the change you have made.
Checklist:
If a new method has been added it should be referenced in cognite.rst in order to generate docs based on its docstring.