-
-
Notifications
You must be signed in to change notification settings - Fork 690
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
Adding support for Gemini-8b and 2.0 #1274
Conversation
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.
👍 Looks good to me! Reviewed everything up to b0dc103 in 29 seconds
More details
- Looked at
49
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
6
drafted comments based on config settings.
1. tests/llm/test_gemini/util.py:3
- Draft comment:
Ensure that the model version 'gemini-1.5-flash-8b' is correctly supported in the codebase. If there are any specific configurations or dependencies for this model, they should be updated accordingly. - Reason this comment was not posted:
Confidence changes required:50%
The PR adds a new model version 'gemini-1.5-flash-8b' in the test utility file. This change seems intentional and aligns with the PR title, which mentions adding support for Gemini-8b.
2. .github/workflows/test.yml:46
- Draft comment:
Ensure that theGEMINI_API_KEY
is correctly set in the repository secrets asGOOGLE_API_KEY
. If not, this will cause the tests to fail. - Reason this comment was not posted:
Confidence changes required:50%
The PR adds a new test step for Gemini in the GitHub Actions workflow. This step is conditioned to run only for Python 3.11, which is consistent with the existing pattern for other tests.
3. instructor/reask.py:109
- Draft comment:
Ensure thatresponse.parts[0].function_call
is always valid and contains the expected attributes (name
andargs
). Consider adding error handling if there's a possibility of these attributes being absent or malformed. - Reason this comment was not posted:
Confidence changes required:50%
The PR adds a new block of code to handle Gemini function calls in thereask_gemini_tools
function. This is consistent with the PR's intent to add support for Gemini-8b and 2.0.
4. tests/llm/test_gemini/util.py:3
- Draft comment:
Use descriptive variable names. For example, renamemodels
togemini_models
for clarity. This applies to other similar instances if any. - Reason this comment was not posted:
Confidence changes required:80%
The variable name 'models' is not descriptive enough. It should indicate what kind of models it is referring to.
5. instructor/reask.py:107
- Draft comment:
Add comments to explain the logic inreask_gemini_tools
function as it is complex. This will help in understanding the code better. - Reason this comment was not posted:
Comment was on unchanged code.
6. .github/workflows/test.yml:46
- Draft comment:
Ensure that if the Gemini tests affect library code, the documentation is updated accordingly. This applies to other similar instances if any. - Reason this comment was not posted:
Confidence changes required:70%
The new test for Gemini in the GitHub Actions workflow should have corresponding documentation updates if it affects library code.
Workflow ID: wflow_UbvwvQ72QiD1angP
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Deploying instructor-py with Cloudflare Pages
|
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.
👍 Looks good to me! Incremental review on 93fccf2 in 32 seconds
More details
- Looked at
16
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. .github/workflows/test.yml:46
- Draft comment:
Add a condition to run Gemini tests only for Python 3.11 to match the PR description intent.
if: matrix.python-version == '3.11'
- Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment suggests adding back a condition that was deliberately removed in the PR. Without additional context about why it was removed, I should assume the author had a valid reason to want Gemini tests to run on all Python versions. The PR rules state not to assume anything about PR description intent. The comment is making assumptions about intent that we can't verify.
Maybe running Gemini tests on all Python versions could cause issues with CI resources or test stability that the commenter is aware of?
Even if there are concerns, they should be explicitly stated rather than assuming intent. The removal of the condition was a deliberate change in the PR.
The comment should be deleted as it assumes PR intent and suggests reverting a deliberate change without clear justification.
2. .github/workflows/test.yml:49
- Draft comment:
Ensure the environment variableGOOGLE_API_KEY
is correct and consistent with the secret key name. If it was intended to beGEMINI_API_KEY
, update it accordingly. - Reason this comment was not posted:
Confidence changes required:80%
The environment variable name was changed fromGEMINI_API_KEY
toGOOGLE_API_KEY
in theRun Gemini Tests
step, but the key in the secrets is stillGOOGLE_API_KEY
. This might be intentional, but it should be checked for consistency.
Workflow ID: wflow_jXr1fRt9tc8hW4Vw
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
This allows for supporting the flash-2.0 models |
Important
Add support for Gemini-8b with updated tests and response handling in
reask_gemini_tools
..github/workflows/test.yml
for Python 3.11.models
intests/llm/test_gemini/util.py
to includegemini-1.5-flash-8b
.reask_gemini_tools
ininstructor/reask.py
to handle newglm.FunctionCall
response parts.This description was created by for 93fccf2. It will automatically update as commits are pushed.