Skip to content
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

vertexai[patch]: support model param #81

Merged
merged 3 commits into from
Mar 26, 2024
Merged

Conversation

baskaryan
Copy link
Contributor

@baskaryan baskaryan commented Mar 21, 2024

Related to langchain-ai/langchain#18975

Didn't go the usual pydantic Field(..., alias="model") route because that would've caused mypy errors for anyone passing in model_name (ie for all existing code).

@lkuligin lkuligin force-pushed the bagatur/support_model_param branch 4 times, most recently from bf56160 to 8b6122f Compare March 24, 2024 11:11
@lkuligin lkuligin force-pushed the bagatur/support_model_param branch from 8b6122f to 5ddc4f0 Compare March 24, 2024 11:14
@@ -55,6 +55,12 @@ class _VertexAIBase(BaseModel):
model_name: Optional[str] = None
"Underlying model name."

@root_validator(pre=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add an @property for model as well?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you know any use case when it's actually consumed by any chaiins or other "external" instances?
if model property is commonly used outside the LLM/ChatModel itself, maybe we should actually deprecate model_name then.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you know any use case when it's actually consumed by any chaiins or other "external" instances? if model property is commonly used outside the LLM/ChatModel itself, maybe we should actually deprecate model_name then.

I think we should use model_name to stay consistent with other provider.
See this issue
#81
@lkuligin @efriis

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll merge this one as is then, and we can add another property later if we decide so.

@lkuligin lkuligin merged commit 73f83ea into main Mar 26, 2024
13 checks passed
@lkuligin lkuligin deleted the bagatur/support_model_param branch March 26, 2024 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants