-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Correctly merge Model.settings
with model_settings
in direct mode
#2980
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
Conversation
@DouweM Do you have any suggestions on how to test this? |
@moritzwilksch Might it be easier to do this in pydantic-ai/pydantic_ai_slim/pydantic_ai/agent/__init__.py Lines 591 to 592 in bfcccba
You could test it using
|
I implemented it there initially but users can also invoke |
@moritzwilksch Hmm, that's right, but then I think we should do the same thing for I'd prefer just one repeated line: model_settings, model_request_parameters = self.prepare_request(model_settings, model_request_parameters) Can you implement that on |
thanks for the pointers, changed. Let me know whether this is missing anything |
ca4ed92
to
5b53535
Compare
5b53535
to
8c436af
Compare
@moritzwilksch Thanks Moritz! |
fixes #2956
by merging the
Model.settings
with the providedmodel_settings
within each model's request method. In an earlier version I adjusted only the direct functions indirect.py
but since users can usemy_model.request(...)
as well, the merging should be the responsibility of theModel
.