-
-
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
feat: add Parallel Tool mode for Vertex AI #1217
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 c582618 in 1 minute and 12 seconds
More details
- Looked at
233
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
8
drafted comments based on config settings.
1. instructor/client_vertexai.py:39
- Draft comment:
Remove the debug print statement or replace it with proper logging.
# print(f"Debug - Model list: {[model.__name__ for model in model_list]}")
- Reason this comment was not posted:
Confidence changes required:80%
The debug print statement is not suitable for production code and should be removed or replaced with proper logging.
2. instructor/client_vertexai.py:30
- Draft comment:
The type hint formodels
should be updated to only includeBaseModel
orlist[BaseModel]
.
def _create_vertexai_tool(models: Union[BaseModel, list[BaseModel]]) -> gm.Tool:
- Reason this comment was not posted:
Confidence changes required:80%
The function_create_vertexai_tool
is designed to handle both single models and lists of models. However, the type hint formodels
includesType
, which is not necessary and could lead to confusion. The function should only acceptBaseModel
orlist[BaseModel]
.
3. instructor/client_vertexai.py:104
- Draft comment:
The type hint formodel
should be updated to only includeBaseModel
orlist[BaseModel]
.
def vertexai_process_response(_kwargs: dict[str, Any], model: Union[BaseModel, list[BaseModel]]):
- Reason this comment was not posted:
Confidence changes required:80%
The functionvertexai_process_response
is designed to handle both single models and lists of models. However, the type hint formodel
includesType
, which is not necessary and could lead to confusion. The function should only acceptBaseModel
orlist[BaseModel]
.
4. instructor/client_vertexai.py:39
- Draft comment:
Debug print statements should be removed or replaced with proper logging.
# print(f"Debug - Model list: {[model.__name__ for model in model_list]}")
- Reason this comment was not posted:
Confidence changes required:80%
The function_create_vertexai_tool
ininstructor/client_vertexai.py
has a debug print statement that should be removed or replaced with proper logging.
5. instructor/client_vertexai.py:13
- Draft comment:
Function_create_gemini_json_schema
should have a return type annotation for clarity and consistency.
def _create_gemini_json_schema(model: BaseModel) -> dict[str, Any]:
- Reason this comment was not posted:
Confidence changes required:80%
The function_create_gemini_json_schema
ininstructor/client_vertexai.py
lacks a return type annotation, which is important for clarity and consistency.
6. instructor/client_vertexai.py:30
- Draft comment:
Function_create_vertexai_tool
should have a return type annotation for clarity and consistency.
def _create_vertexai_tool(models: Union[BaseModel, list[BaseModel], Type]) -> gm.Tool:
- Reason this comment was not posted:
Confidence changes required:80%
The function_create_vertexai_tool
ininstructor/client_vertexai.py
lacks a return type annotation, which is important for clarity and consistency.
7. instructor/client_vertexai.py:104
- Draft comment:
Functionvertexai_process_response
should have a return type annotation for clarity and consistency.
def vertexai_process_response(_kwargs: dict[str, Any], model: Union[BaseModel, list[BaseModel], Type]) -> tuple[list[gm.Content], list[gm.Tool], ToolConfig]:
- Reason this comment was not posted:
Confidence changes required:80%
The functionvertexai_process_response
ininstructor/client_vertexai.py
lacks a return type annotation, which is important for clarity and consistency.
8. instructor/dsl/parallel.py:50
- Draft comment:
Functionfrom_response
should have a return type annotation for clarity and consistency.
def from_response(
- Reason this comment was not posted:
Confidence changes required:80%
The functionfrom_response
ininstructor/dsl/parallel.py
lacks a return type annotation, which is important for clarity and consistency.
Workflow ID: wflow_tx9ZvP1F3sMzVhxR
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
c582618
to
41c1a78
Compare
Verified that all vertex tests are passing locally and updated the docs
|
This PR intends to add parallel tool mode for Vertex AI.
That is done by adding new mode
VERTEXAI_PARALLEL_TOOLS
.Important
Adds
VERTEXAI_PARALLEL_TOOLS
mode for parallel tool processing in Vertex AI, with updates to model handling and response processing.VERTEXAI_PARALLEL_TOOLS
mode inmode.py
.from_vertexai()
inclient_vertexai.py
to support the new mode.handle_vertexai_parallel_tools()
inprocess_response.py
to process responses in parallel.VertexAIParallelBase
inparallel.py
for handling parallel responses.VertexAIParallelModel()
inparallel.py
to create instances ofVertexAIParallelBase
._create_vertexai_tool()
inclient_vertexai.py
to handle multiple models.vertexai_process_response()
inclient_vertexai.py
to support parallel tools._create_gemini_json_schema()
inclient_vertexai.py
.This description was created by for c582618. It will automatically update as commits are pushed.