Skip to content
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

Deprecate redundant field in ModelObservationContext #2227

Conversation

ThomasVitale
Copy link
Contributor

@ThomasVitale ThomasVitale commented Feb 12, 2025

Each ModelObservationContext takes both a request object (e.g. Prompt) and an options object (e.g. ChatOptions). However, the options are already included in the request object. This PR deprecates the additional field, which will be removed in a subsequent release.

The reason why the extra field was there in the first place was due to the model implementations not handling request options correctly, requiring a dedicated setter. We started fixing the model implementations now, so we are deprecating the extra field, and we'll remove it in the next release, once we have completed the implementation of a fix for all model implementations.

Each ModelObservationContext takes both a request object (e.g. Prompt) and an options object (e.g. ChatOptions). However, the options are already included in the request object. This PR deprecates the additional field, which will be removed in a subsequent release.

The reason why the extra field was there in the first place was due to the model implementations not handling request options correctly, requiring a dedicated setter. We started fixing the model implementations now, so we are deprecating te extra field, and we'll remove it in the next release, once we have completed the implementation of a fix for all model implementations.

Signed-off-by: Thomas Vitale <[email protected]>
@ThomasVitale ThomasVitale marked this pull request as ready for review February 12, 2025 14:38
@tzolov tzolov added this to the 1.0.0-M6 milestone Feb 13, 2025
@tzolov tzolov self-assigned this Feb 13, 2025
@tzolov
Copy link
Contributor

tzolov commented Feb 13, 2025

rebased and merged at a307a2b

@tzolov tzolov closed this Feb 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants