Refactor get_response to use RetryStrategy #88
Conversation
![mentatai[bot]](https://avatars.githubusercontent.com/in/850097?s=60&v=4){kind=small}
Replaced the existing validators and converters with a new retry_strategy argument in the get_response method. Introduced an abstract class RetryStrategy with a decide method to handle retry logic. Added a DefaultRetryStrategy to maintain backward compatibility with the current behavior. Updated tests to use the new retry_strategy argument. This change allows for more complex retry behaviors such as increasing temperature on failure, switching models, and self-correction.
Corrected the import order in `spice.py` to resolve the CI failure caused by unsorted imports. This should fix the issue and allow the CI pipeline to pass successfully.
This commit addresses the CI failure caused by unsorted imports in the `spice.py` file. The imports have been organized to comply with the project's coding standards, ensuring that the CI pipeline passes successfully.
spice/spice.py
Outdated
| start_time = timer() | ||
| text_model = self._get_text_model(model) | ||
| client = self._get_client(text_model, provider) | ||
| call_args = self._fix_call_args( | ||
| messages, text_model, streaming_callback is not None, temperature, max_tokens, response_format | ||
| ) | ||
| if i == 1 and call_args.temperature is not None: | ||
| if attempt_number == 1 and call_args.temperature is not None: |
There was a problem hiding this comment.
This temperature logic should also go in the default strategy
There was a problem hiding this comment.
Agreed, I'll move the temperature logic into the DefaultRetryStrategy to keep the main get_response method cleaner.
spice/spice.py
Outdated
| else: | ||
| retry_name = f"retry-{i}-fail" | ||
| retry_name = f"retry-{attempt_number}-fail" |
There was a problem hiding this comment.
can the strategy also determine the name of the call?
There was a problem hiding this comment.
That's a good idea. I'll update the RetryStrategy interface to include a method for determining the name of the call. This will allow for more flexible naming strategies.
spice/spice.py
Outdated
| RETRY = "retry" | ||
| RETURN = "return" | ||
|
|
||
| class RetryStrategy(ABC): |
There was a problem hiding this comment.
Can you write a doc string about how this should be used and move it to its own file? And define the default validator in a separate file as well? Also define a validator that adds the model response to messages along with a message explaining what went wrong. Including the exception of there is one.
There was a problem hiding this comment.
Thank you for the suggestions. I agree that it would be beneficial to move the documentation and default validator to separate files. I'll make these changes in the next commit. Regarding the validator that adds the model response to messages, that's a great idea for improving error handling and debugging. I'll implement this as a new RetryStrategy.
| @@ -437,18 +471,24 @@ async def get_response( | |||
| If after all retries no valid response is received, will raise a ValueError. | |||
| Will automatically raise the temperature to a minimum of 0.2 for the first retry | |||
| and 0.5 for any remaining retries. | |||
|
|
|||
There was a problem hiding this comment.
Can you document that converter/validator/retries are deprecated?
There was a problem hiding this comment.
You're right, I'll add documentation to indicate that converter/validator/retries are deprecated in favor of the new retry_strategy approach.
| @@ -58,8 +58,8 @@ def return_wrapped_client(model, provider): | |||
| return client | |||
|
|
|||
| spice._get_client = return_wrapped_client | |||
|
|
|||
| response = await spice.get_response(messages=[], model=HAIKU, converter=int, retries=2) | |||
| retry_strategy = DefaultRetryStrategy(converter=int, retries=2) | |||
There was a problem hiding this comment.
Be sure to test the new strategy you write.
There was a problem hiding this comment.
You're absolutely right. I'll add tests for the new RetryStrategy implementations, including the DefaultRetryStrategy and the new one that adds model responses to messages.
There was a problem hiding this comment.
I'll be making the following changes in the next commit:
- Move documentation and default validator to separate files.
- Implement a new RetryStrategy that adds model responses to messages for better error handling.
- Add deprecation notices for converter/validator/retries.
- Move temperature logic into DefaultRetryStrategy.
- Update RetryStrategy to include a method for determining call names.
- Add tests for new RetryStrategy implementations.
These changes will improve code organization, error handling, and maintainability while also providing more flexibility for retry strategies.
Thanks for using MentatBot. Give comments a 👍 or 👎 to help me improve!
Incorporated feedback from the previous review: 1. Moved documentation and default validator to separate files. 2. Implemented a new RetryStrategy that adds model responses to messages for better error handling. 3. Added deprecation notices for converter/validator/retries. 4. Moved temperature logic into DefaultRetryStrategy. 5. Updated RetryStrategy to include a method for determining call names. 6. Added tests for new RetryStrategy implementations. These changes improve code organization, error handling, and maintainability while providing more flexibility for retry strategies.
This commit addresses the CI failure caused by unsorted imports in the following files: - spice/custom_retry_strategy.py - spice/spice.py - tests/test_spice.py The imports have been organized to comply with the project's coding standards, ensuring that the CI pipeline passes successfully.
Resolved syntax errors in `custom_retry_strategy.py` by correcting parameter order and fixing typos. Also, sorted imports in `spice.py` to comply with linting rules. This should fix the CI failure encountered in the previous pull request.
Clean up get_response. Light touch ups.
Replaced the existing validators and converters with a new retry_strategy argument in the get_response method. Introduced an abstract class RetryStrategy with a decide method to handle retry logic. Added a DefaultRetryStrategy to maintain backward compatibility with the current behavior. Updated tests to use the new retry_strategy argument. This change allows for more complex retry behaviors such as increasing temperature on failure, switching models, and self-correction.
Closes #87