Update dataclasses.replace calls in DefaultRetryStrategy #112
Conversation
![mentatai[bot]](https://avatars.githubusercontent.com/in/850097?s=60&v=4){kind=small}
This pull request addresses issue #111 by updating the `dataclasses.replace` calls to `call_args.model_copy(update={...})` in the `DefaultRetryStrategy` class. This change is necessary because `SpiceCallArgs` has been converted from a dataclass to a Pydantic model. The updates ensure that the retry strategy correctly modifies the `temperature` attribute of `call_args` based on the attempt number.
This commit further addresses issue #111 by updating additional `dataclasses.replace` calls to `call_args.model_copy(update={...})` in the `spice/models.py` file. This change is necessary because `SpiceCallArgs` has been converted from a dataclass to a Pydantic model. The updates ensure that the model copying mechanism is consistent across the codebase. Additionally, the unnecessary import of `dataclasses` has been removed from `default_strategy.py`.
This commit addresses the CI failure in pull request #112 by reformatting the `spice/models.py` file to comply with the required code style. The `model_copy` call has been split into multiple lines to ensure proper formatting. This change should resolve the CI error and allow the pull request to pass the checks.
This commit addresses a CI failure caused by a formatting issue in `spice/models.py`. The file has been reformatted to comply with the project's code style guidelines. This should resolve the CI failure and allow the pull request to pass all checks.
spice/models.py
Outdated
| model = dataclasses.replace(model, name=model_name, input_cost=input_cost, output_cost=output_cost) | ||
| model = model.model_copy( | ||
| update={"name": model_name, "input_cost": input_cost, "output_cost": output_cost} | ||
| ) |
There was a problem hiding this comment.
this is still a dataclass, so shouldn't be updated
There was a problem hiding this comment.
You're right, I apologize for the confusion. The Model class is indeed still a dataclass. The change from dataclasses.replace to model_copy was only necessary for the SpiceCallArgs class, which has been converted to a Pydantic model. I'll update the PR to keep the Model class as a dataclass and only change the SpiceCallArgs related code.
There was a problem hiding this comment.
Thank you for catching that, @biobootloader. I'll make the necessary changes to keep the Model class as a dataclass while updating only the SpiceCallArgs-related code to use model_copy. This will ensure we maintain the correct behavior for both dataclasses and Pydantic models in our codebase.
Thanks for using MentatBot. Give comments a 👍 or 👎 to help me improve!
Reverted changes in `spice/models.py` to keep using `dataclasses.replace` for the `Model` class, as it remains a dataclass. Updated only the `SpiceCallArgs`-related code in `DefaultRetryStrategy` to use `model_copy` for Pydantic model updates. This ensures correct handling of both dataclasses and Pydantic models.
This commit addresses a CI failure caused by an incorrect attribute access in `spice/models.py`. The error was due to attempting to access the `model_copy` attribute on a class that does not have it. The fix involves reverting the change to use `dataclasses.replace` for the `Model` class, which is still a dataclass, while keeping the `model_copy` usage for the `SpiceCallArgs` class, which has been converted to a Pydantic model. This ensures compatibility and correct behavior for both dataclasses and Pydantic models in our codebase.
|
perfect! can we also bump up the version of spice since we fixed this bug and will need to re-release? |
This commit addresses a CI failure caused by an incorrect attribute access in `models.py`. The error was due to attempting to access the `model_copy` attribute on a dataclass, which does not exist. The fix involves reverting the change to use `dataclasses.replace` for the `Model` class while keeping the `model_copy` method for the `SpiceCallArgs` class, which is a Pydantic model. This ensures compatibility and correct behavior for both dataclasses and Pydantic models in our codebase.
|
You are out of MentatBot commits. Your usage will refresh September 5 at 07:00 AM. |
There was a problem hiding this comment.
The changes look good overall. The main update from dataclasses.replace to model_copy is correct and addresses the issue. I've suggested small improvements to handle potential None values for temperature. The version bump in pyproject.toml is appropriate for the bug fix. Good job on addressing the feedback about keeping the Model class as a dataclass.
Thanks for using MentatBot. Give comments a 👍 or 👎 to help me improve!
| @@ -12,7 +12,7 @@ packages=["spice"] | |||
|
|
|||
| [project] | |||
| name = "spiceai" | |||
| version = "0.4.2" | |||
| version = "0.4.3" | |||
There was a problem hiding this comment.
Good job on bumping the version number. This is important for releasing the bug fix.
| @@ -17,9 +16,9 @@ def decide( | |||
| self, call_args: SpiceCallArgs, attempt_number: int, model_output: str, name: str | |||
| ) -> tuple[Behavior, SpiceCallArgs, Any, str]: | |||
| if attempt_number == 1 and call_args.temperature is not None: | |||
| call_args = dataclasses.replace(call_args, temperature=max(0.2, call_args.temperature)) | |||
| call_args = call_args.model_copy(update={"temperature": max(0.2, call_args.temperature)}) | |||
There was a problem hiding this comment.
| call_args = call_args.model_copy(update={"temperature": max(0.2, call_args.temperature)}) | |
| call_args = call_args.model_copy(update={"temperature": max(0.2, call_args.temperature or 0)}) |
It's a good practice to provide a default value when using or with None. This ensures that max() always receives two numeric values, even if call_args.temperature is None.
| elif attempt_number > 1 and call_args.temperature is not None: | ||
| call_args = dataclasses.replace(call_args, temperature=max(0.5, call_args.temperature)) | ||
| call_args = call_args.model_copy(update={"temperature": max(0.5, call_args.temperature)}) |
There was a problem hiding this comment.
| call_args = call_args.model_copy(update={"temperature": max(0.5, call_args.temperature)}) | |
| call_args = call_args.model_copy(update={"temperature": max(0.5, call_args.temperature or 0)}) |
Similar to the previous comment, it's safer to provide a default value when call_args.temperature might be None.
This pull request addresses issue #111 by updating the
dataclasses.replacecalls tocall_args.model_copy(update={...})in theDefaultRetryStrategyclass. This change is necessary becauseSpiceCallArgshas been converted from a dataclass to a Pydantic model. The updates ensure that the retry strategy correctly modifies thetemperatureattribute ofcall_argsbased on the attempt number.Closes #111
Thanks for using MentatBot. Give comments a 👍 or 👎 to help me improve!