-
-
Notifications
You must be signed in to change notification settings - Fork 684
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
Check vai parallel pipeline #1265
Conversation
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! Reviewed everything up to 6b5c90e in 2 minutes and 49 seconds
More details
- Looked at
398
lines of code in6
files - Skipped
0
files when reviewing. - Skipped posting
7
drafted comments based on config settings.
1. instructor/client_vertexai.py:39
- Draft comment:
Remove debug print statements from production code to avoid unnecessary console output.
# print(f"Debug - Model list: {[model.__name__ for model in model_list]}")
- Reason this comment was not posted:
Confidence changes required:50%
The debug print statement should be removed from production code.
2. instructor/client_vertexai.py:16
- Draft comment:
Consider providing a more descriptive error message to help users understand the issue.
raise TypeError(f"Expected a concrete model class, but received a type hint: {model}")
- Reason this comment was not posted:
Confidence changes required:50%
The error message should be more descriptive to help users understand the issue.
3. instructor/client_vertexai.py:145
- Draft comment:
Update the assertion message to match the allowed modes.
}, "Mode must be one of VERTEXAI_PARALLEL_TOOLS, VERTEXAI_TOOLS, or VERTEXAI_JSON"
- Reason this comment was not posted:
Confidence changes required:50%
The assertion message should match the allowed modes.
4. instructor/client_vertexai.py:39
- Draft comment:
Remove debug print statements from production code. Consider using a logging framework if needed for debugging purposes. - Reason this comment was not posted:
Confidence changes required:80%
The function_create_vertexai_tool
ininstructor/client_vertexai.py
has a debug print statement which is not ideal for production code. It should be removed or replaced with proper logging.
5. instructor/client_vertexai.py:13
- Draft comment:
Add a descriptive comment explaining the purpose and functionality of_create_gemini_json_schema
. This will help in understanding the code better, especially given its complexity. - Reason this comment was not posted:
Confidence changes required:70%
The function_create_gemini_json_schema
ininstructor/client_vertexai.py
lacks a descriptive comment explaining its purpose and functionality. Given its complexity, a comment would be beneficial.
6. instructor/client_vertexai.py:141
- Draft comment:
Assertions should have descriptive error messages. Consider adding a message to the assertion invertexai_process_response
. - Reason this comment was not posted:
Comment was on unchanged code.
7. docs/concepts/parallel.md:5
- Draft comment:
Ensure new markdown files are added tomkdocs.yml
to be included in the documentation. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_JuvGVU2qqo3Vm6Yb
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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 31ef97a in 43 seconds
More details
- Looked at
64
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. instructor/client_vertexai.py:3
- Draft comment:
Type
is used in the function signatures but is not imported. Re-addType
to the imports to avoid a NameError. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. instructor/client_vertexai.py:37
- Draft comment:
Consider using a logging statement instead of a print statement for debugging purposes, or remove it if it's no longer needed. - Reason this comment was not posted:
Confidence changes required:50%
The debug print statement was removed from_create_vertexai_tool
. If this was used for debugging purposes, it should be replaced with a proper logging statement or removed if no longer needed.
3. instructor/process_response.py:502
- Draft comment:
VertexAIParallelModel
is used in the return type but is not imported. Re-addVertexAIParallelModel
to the imports to avoid a NameError. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_xdRInFYRg3GWMWci
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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 971cedd in 18 seconds
More details
- Looked at
15
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. instructor/client_vertexai.py:104
- Draft comment:
The closing parenthesis should be aligned with the opening parenthesis for better readability.
):
- Reason this comment was not posted:
Confidence changes required:10%
The functionvertexai_process_response
has a minor formatting issue with the closing parenthesis. It should be aligned with the opening parenthesis for better readability.
2. instructor/client_vertexai.py:105
- Draft comment:
Add an assertion to check if 'messages' is present in_kwargs
before popping it to avoid potential KeyError. - Reason this comment was not posted:
Confidence changes required:80%
The functionvertexai_process_response
has a parameter_kwargs
which is a dictionary, and it pops the keymessages
from it. The assertion for the presence ofmessages
in_kwargs
is missing, which could lead to a KeyError ifmessages
is not present.
3. instructor/client_vertexai.py:104
- Draft comment:
Add an assertion to check if 'messages' is present in_kwargs
before popping it to avoid potential KeyError. - Reason this comment was not posted:
Confidence changes required:80%
The functionvertexai_process_response
has a parameter_kwargs
which is a dictionary, and it pops the keymessages
from it. The assertion for the presence ofmessages
in_kwargs
is missing, which could lead to a KeyError ifmessages
is not present.
Workflow ID: wflow_5K3yEeY5vyQtMROI
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Important
Adds support for parallel function calling in Vertex AI, updating documentation, implementation, and tests to reflect this new feature.
docs/concepts/parallel.md
to include Vertex AI examples and usage.VertexAIParallelBase
class ininstructor/dsl/parallel.py
for handling Vertex AI parallel responses._create_vertexai_tool()
inclient_vertexai.py
to handle multiple models.vertexai_process_response()
inclient_vertexai.py
to support parallel tools.VERTEXAI_PARALLEL_TOOLS
mode inmode.py
.handle_vertexai_parallel_tools()
inprocess_response.py
to process responses in parallel mode.test_modes.py
to test new parallel functionality with Vertex AI models.This description was created by for 971cedd. It will automatically update as commits are pushed.