Skip to content

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

Merged
merged 2 commits into from
Apr 16, 2025
Merged

LLM aggregator params #1588

merged 2 commits into from
Apr 16, 2025

Conversation

aconchillo
Copy link
Contributor

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.

Copy link

codecov bot commented Apr 14, 2025

Comment on lines -123 to -124
user_kwargs: Mapping[str, Any] = {},
assistant_kwargs: Mapping[str, Any] = {},
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changelog updated.

Comment on lines -874 to -875
user_kwargs: Mapping[str, Any] = {},
assistant_kwargs: Mapping[str, Any] = {},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Copy link
Contributor

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.

@aconchillo aconchillo force-pushed the aleix/llm-aggregator-params branch from e2adc3f to 6a209c8 Compare April 16, 2025 21:38
Copy link
Contributor

@filipi87 filipi87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@aconchillo aconchillo force-pushed the aleix/llm-aggregator-params branch from 6a209c8 to f385cc0 Compare April 16, 2025 22:19
@aconchillo aconchillo merged commit f6f01ea into main Apr 16, 2025
6 checks passed
@aconchillo aconchillo deleted the aleix/llm-aggregator-params branch April 16, 2025 22:25
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