-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
cohere 5.0.0 support #38465
cohere 5.0.0 support #38465
Conversation
0f2159d
to
3173797
Compare
Seems that
|
|
||
def create_embeddings( | ||
self, texts: list[str], model: str = "embed-multilingual-v2.0" | ||
) -> list[list[float]]: | ||
response = self.get_conn.embed(texts=texts, model=model) | ||
) -> list[list[float]] | cohere.EmbedByTypeResponseEmbeddings: |
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 return type should be just EmbedResponse
ref. Is there any case where we will still return list[list[float]]?
|
||
- additional_query_parameters: typing.Dict[str, typing.Any]. A dictionary containing additional parameters to spread into the request's query parameters dict | ||
|
||
- additional_body_parameters: typing.Dict[str, typing.Any]. A dictionary containing additional parameters to spread into the request's body parameters dict |
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.
It would be nice to point to original docs here, in case they change in future releases.
return CohereHook( | ||
conn_id=self.conn_id, | ||
timeout=self.timeout, | ||
max_retries=self.max_retries, |
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.
Should we use max_retries=self.max_retries
in the same PR we are deprecating it? :)
self.request_options = ( | ||
{"max_retries": self.max_retries} | ||
if self.request_options is None | ||
else self.request_options.update({"max_retries": self.max_retries}) |
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.
nit: we generally use if/else in this manner to conditionally assign values, which is the intent of if
clause here but in else
you are updating the self.request_options
dict. Which to me seems out of place. It would be much better to have a simplified version of the code here.
self.request_options = ( | ||
{"max_retries": self.max_retries} | ||
if self.request_options is None | ||
else self.request_options.update({"max_retries": self.max_retries}) |
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 pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
@okirialbert Do you wish to continue with this change? |
Yes I'll close it. I'll submit a PR for this with the required changes. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
This PR adds support for Cohere 5.0.0 update
Relates to #38349
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rst
or{issue_number}.significant.rst
, in newsfragments.