-
Notifications
You must be signed in to change notification settings - Fork 723
LLM aggregator params #1588
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
LLM aggregator params #1588
Conversation
user_kwargs: Mapping[str, Any] = {}, | ||
assistant_kwargs: Mapping[str, Any] = {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the changelog, we mention that these parameters are deprecated, but here we are removing them.
Is that intentional ? I think we should fix either the changelog or here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooops, they are not deprecated. They are just removed. No one has ever used those anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changelog updated.
user_kwargs: Mapping[str, Any] = {}, | ||
assistant_kwargs: Mapping[str, Any] = {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And in all the other LLMs actually.
e2adc3f
to
6a209c8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
6a209c8
to
f385cc0
Compare
Please describe the changes in your PR. If it is addressing an issue, please reference that as well.
We replace kwargs with actual classes which will make it easier to set parameters and allow users to configure the aggregators.