Skip to content

Refactor LLM settings #217

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

marginal23326
Copy link
Contributor

@marginal23326 marginal23326 commented Feb 1, 2025

I moved all LLM provider configs (base url, default model names, etc.), into a single dict PROVIDER_CONFIGS.

This should clean up utils.py (especially the get_llm_model function), and make it easier to add new providers.

@CLAassistant
Copy link

CLAassistant commented Feb 2, 2025

CLA assistant check
All committers have signed the CLA.

@marginal23326 marginal23326 force-pushed the refactor/llm-config-cleanup branch 7 times, most recently from 90c4e1c to c767da2 Compare February 8, 2025 22:18
@vvincent1234
Copy link
Contributor

hello, thanks for your PR. refactor PRs need to be reviewed later, because it gets time to test all over

@marginal23326
Copy link
Contributor Author

hello, thanks for your PR. refactor PRs need to be reviewed later, because it gets time to test all over

Got it, thanks for the notice.

@marginal23326 marginal23326 force-pushed the refactor/llm-config-cleanup branch 7 times, most recently from 5b366f2 to e90c987 Compare February 15, 2025 09:25
@marginal23326 marginal23326 force-pushed the refactor/llm-config-cleanup branch 2 times, most recently from 1213538 to be27a1b Compare February 20, 2025 21:33
@marginal23326 marginal23326 force-pushed the refactor/llm-config-cleanup branch 3 times, most recently from 0819d82 to 4a64e7b Compare March 18, 2025 02:17
@marginal23326 marginal23326 changed the title Refactor LLM settings and error handling Refactor LLM settings Mar 18, 2025
@marginal23326 marginal23326 force-pushed the refactor/llm-config-cleanup branch 3 times, most recently from cac4bc3 to 8b98efb Compare March 18, 2025 13:42
@marginal23326 marginal23326 force-pushed the refactor/llm-config-cleanup branch 4 times, most recently from 6060152 to 3af76d0 Compare March 27, 2025 17:45
@marginal23326
Copy link
Contributor Author

Hi, @warmshao.

Could you review this PR when possible?

Thanks

@marginal23326 marginal23326 force-pushed the refactor/llm-config-cleanup branch 4 times, most recently from 7910c5f to a88be4b Compare April 2, 2025 02:45
@marginal23326 marginal23326 force-pushed the refactor/llm-config-cleanup branch from a88be4b to ffe3de5 Compare April 12, 2025 20:34
@warmshao
Copy link
Collaborator

Thank you @marginal23326 for this well-organized refactoring of the LLM settings. Centralizing the provider configurations into a single dictionary is a great step towards improving code maintainability and simplifying the addition of new providers. This change should make the get_llm_model function much cleaner and easier to manage.

The modifications in the test suite and environment file also seem well-aligned with the refactoring goals. We appreciate your contribution, and the maintainers will review the code shortly.

@marginal23326 marginal23326 force-pushed the refactor/llm-config-cleanup branch from ffe3de5 to 2e2db61 Compare April 29, 2025 14: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.

4 participants