feat(cohere): add timeout and max_retries to chat generator#2873
Conversation
7c13187 to
f961fe7
Compare
Keyur-S-Patel
left a comment
There was a problem hiding this comment.
Gentle reminder about pr review @julian-risch
|
@davidsbatista can you look up this one, should be quick review |
julian-risch
left a comment
There was a problem hiding this comment.
@Keyur-S-Patel Thank you for opening this pull request. The code changes look good to me! 👍 I looked into the Cohere SDK and confirmed that when using timeout and max_retries at the same time, for example CohereChatGenerator(timeout=30.0, max_retries=5), the SDK stores the timeout passed via client_kwargs and passes it to each httpx_client.request(..., timeout=30) done with the custom client.
Thank you also for your patience with regard to this review. I'd like to assure you that we do our best to review every community PR in a timely manner. We auto-rotate reviews and there is no need to tag individual team members in PR comments. Thank you!
|
Tysm for reviewing @julian-risch |
Related Issues
Proposed Changes:
timeoutandmax_retriestoCohereChatGenerator.__init__as keyword-only params (aligned with Anthropic pattern).httpxtransport injection:HTTPTransport(retries=max_retries)AsyncHTTPTransport(retries=max_retries)httpx_clientintoClientV2/AsyncClientV2whenmax_retriesis set.**kwargsmerged intogeneration_kwargs).to_dict/from_dictexpectations fortimeoutandmax_retries.CohereGeneratortests to reflect current inherited behavior (generation_kwargsinstead of removedmodel_parameters).How did you test it?
hatch run test:unit tests/test_chat_generator.pyhatch run test:unit tests/test_generator.pyhatch run test:unit tests(full Cohere unit suite)All unit tests passed on this branch.
Notes for the reviewer
max_retriesis implemented viahttpxtransport retries because directmax_retriesconstructor support onClientV2is not consistent across environments/versions.tests/test_generator.pyfor currentCohereGeneratorbehavior.Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:.