-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Bug fix for trailing slash error when using LiteLLM provider #4275
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
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.
Would it make sense to use the URL constructor (e.g., new URL(pathSegment, baseUrl).href) for joining the base URL and path? It inherently handles scenarios with or without trailing slashes on the baseUrl, potentially removing the need for this manual normalization and leading to a cleaner implementation.
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.
I like it, I'll make the change
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.
Hey @kcwhite, I'm requesting changes to restore the unintentionally deleted .rooignore and .roomodes files.
|
Hey @kcwhite, |
daniel-lxs
left a comment
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.
Perfect, thank you for addressing the review comments.
Looks good to me now!
|
Cleaned up the history a bit so we can review this easily, since you had a bunch of unrelated commits from main on your PR. |
Sorry Daniel, I'm educating myself on the proper workflow for open source contributions. My future commits should be cleaner since I will rebase rather than merge. |
|
@kcwhite No worries, just wanted to get your PR ready for review quickly. We all learn at some point. Thank you and sorry. |
|
Blunt is my native tongue, you were hardly blunt. 😀 My team prefers merges, fewer conflicts, so I'm having to re-educate myself to use rebase again. |
Related GitHub Issue
Closes: #4273
Description
This PR fixes an issue where the LiteLLM model fetcher would fail when the base URL contained a trailing slash. The implementation:
The fix is minimal and focused on ensuring that URLs are properly formatted regardless of user input format, preventing the creation of invalid URLs with double slashes (e.g., "http://localhost:4000//v1/model/info").
Test Procedure
Unit Tests: Added a specific test case in
src/api/providers/fetchers/__tests__/litellm.test.tsthat verifies base URLs with trailing slashes are handled correctly.Manual Testing:
To Reproduce the Original Issue:
Type of Change
srcor test files.Pre-Submission Checklist
npm run lint).console.log) has been removed.pnpm test).mainbranch.npm run changesetif this PR includes user-facing changes or dependency updates.Screenshots / Videos
N/A - This is a backend fix with no UI changes.
Documentation Updates
Additional Notes
This is a small but important fix that ensures users can successfully connect to LiteLLM servers regardless of how they format the base URL in their configuration.
Get in Touch
Discord: chuck_33620
Important
Fixes URL formatting issue in LiteLLM provider by normalizing base URL to remove trailing slashes, preventing invalid URLs.
getLiteLLMModels()inlitellm.tsby normalizing base URL to remove trailing slashes.litellm.test.tsto verify handling of base URLs with trailing slashes.litellm.tsexplaining URL normalization step.This description was created by
for 9d59c76cc1f530329c2c336c8f7a535fb5bfc3c2. You can customize this summary. It will automatically update as commits are pushed.