Skip to content

fix(llm): ensure base_url has protocol prefix for model info fetch when using LiteLLM #7782

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 14 commits into from
Apr 10, 2025

Conversation

NarwhalChen
Copy link
Contributor

@NarwhalChen NarwhalChen commented Apr 9, 2025

  • This change is worth documenting at https://docs.all-hands.dev/
  • Include this change in the Release Notes. If checked, you must provide an end-user friendly description for your change below

End-user friendly description of the problem this fixes or functionality that this introduces.
Fixes a bug where users could accidentally provide a base URL without the required protocol (e.g., http:// or https://) when using model from LiteLLM. This caused runtime failures in LLM model info fetching


Give a summary of what the PR does, explaining any non-trivial design decisions.

  1. In LLM.init_model_info(), I prepend http:// to base_url if the user has forgotten to include a protocol when using LiteLLM model. This helps avoid confusing runtime UnsupportedProtocol or ConnectError exceptions.

  2. Added a unit test to confirm that missing protocols are corrected before the HTTP request is sent.

  3. Fix i18n issue SetupPaymenyModal


Link of any specific issues this addresses.
Resolves #7760

@enyst
Copy link
Collaborator

enyst commented Apr 9, 2025

Thank you!

You may want to run make build in the project directory, it should install pre-commit hooks and then make linting fixes as you commit, so you don't have to investigate the CI failure.

@NarwhalChen
Copy link
Contributor Author

Thank you!

You may want to run make build in the project directory, it should install pre-commit hooks and then make linting fixes as you commit, so you don't have to investigate the CI failure.

Thanks! But I found CI failure is because of I forget the base_url sometimes will be None and cannot use stripe..

Copy link
Member

@amanape amanape left a comment

Choose a reason for hiding this comment

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

LGTM, though it would also be good practice to trim the string from the frontend before it is sent to the server. See

const finalLlmBaseUrl = shouldHandleSpecialSaasCase
? undefined
: llmBaseUrl;

@amanape amanape self-assigned this Apr 10, 2025
@NarwhalChen
Copy link
Contributor Author

LGTM, though it would also be good practice to trim the string from the frontend before it is sent to the server. See

const finalLlmBaseUrl = shouldHandleSpecialSaasCase
? undefined
: llmBaseUrl;

Thanks! At first I thought the problem was caused by the initialization according to config, so I neglected the frontend when reproducing the bug. But maybe it's better to be here at llm now? This allows it to handle frontend sending and initialization.

@NarwhalChen NarwhalChen requested a review from amanape April 10, 2025 15:11
Copy link
Member

@amanape amanape left a comment

Choose a reason for hiding this comment

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

Nice, thanks a lot for resolving this issue!

@amanape amanape merged commit 513f7ab into All-Hands-AI:main Apr 10, 2025
17 checks passed
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.

[Bug]: App errors if base URL contains whitespace
3 participants