-
Notifications
You must be signed in to change notification settings - Fork 80
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
fix identifying params #16
Conversation
return { | ||
**{"model_kwargs": _model_kwargs}, | ||
"model_id": self.model_id, | ||
"provider": self._get_provider(), | ||
"stream": self.streaming, | ||
"guardrails": self.guardrails, | ||
**_model_kwargs, | ||
} |
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.
With this change, would we need to call _get_provider
in any of the other functions or can just use self.provider
? For example:
https://github.com/langchain-ai/langchain-aws/blob/main/libs/aws/langchain_aws/llms/bedrock.py#L462
provider = self._get_provider() |
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.
I think fixed! This might have been a comment from before I changed self.provider
to self._get_provider()
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.
Also just calling out - this moves the model_kwargs into a flat structure of invocation params to match most other model providers. Let me know if that sounds good to you.
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.
I think I misunderstood the question. Still need to use self._get_provider()
everywhere because provider isn't always set.
Although Likely worth adding a root validator that populates that field based on model string.
if k := self.provider_stop_sequence_key_name_map.get(provider): | ||
_model_kwargs[k] = stop | ||
|
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.
This is great. Ty.
renders in langsmith: https://smith.langchain.com/public/35edf4ec-dabb-4560-b83e-508d6185b9fa/r?runtab=2