-
Notifications
You must be signed in to change notification settings - Fork 19.8k
fix(ollama): Respect scheme-less base_url
#34042
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
base: master
Are you sure you want to change the base?
fix(ollama): Respect scheme-less base_url
#34042
Conversation
CodSpeed Performance ReportMerging #34042 will not alter performanceComparing Summary
Footnotes
|
base_url
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.
Pull request overview
This PR fixes issue #33986 by adding support for scheme-less base_url values in the Ollama integration. The implementation normalizes scheme-less inputs (e.g., ollama:11434 or user:password@host:port) by defaulting to http:// scheme when appropriate.
Key changes:
- Enhanced
parse_url_with_auth()function to detect and handle scheme-less URLs - Added URL reconstruction logic to ensure consistent output format
- Extended test coverage with two new test cases for scheme-less scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
libs/partners/ollama/langchain_ollama/_utils.py |
Modified parse_url_with_auth() to detect scheme-less URLs and add default http:// scheme, plus reconstruct URLs consistently |
libs/partners/ollama/tests/unit_tests/test_auth.py |
Added tests for scheme-less host:port and scheme-less URLs with credentials |
| cleaned_netloc = parsed.hostname or "" | ||
| if parsed.port: | ||
| cleaned_netloc += f":{parsed.port}" | ||
| cleaned_url = f"{parsed.scheme}://{cleaned_netloc}" | ||
| if parsed.path: | ||
| cleaned_url += parsed.path | ||
| if parsed.query: | ||
| cleaned_url += f"?{parsed.query}" | ||
| if parsed.fragment: | ||
| cleaned_url += f"#{parsed.fragment}" | ||
| return cleaned_url, None |
Copilot
AI
Nov 24, 2025
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.
Code duplication: The URL reconstruction logic (lines 80-90 and 103-113) is duplicated. Consider extracting this into a helper function to improve maintainability and reduce the risk of inconsistencies.
Example:
def _reconstruct_url(parsed: ParseResult) -> str:
"""Reconstruct URL from parsed components without userinfo."""
cleaned_netloc = parsed.hostname or ""
if parsed.port:
cleaned_netloc += f":{parsed.port}"
cleaned_url = f"{parsed.scheme}://{cleaned_netloc}"
if parsed.path:
cleaned_url += parsed.path
if parsed.query:
cleaned_url += f"?{parsed.query}"
if parsed.fragment:
cleaned_url += f"#{parsed.fragment}"
return cleaned_urlThen use it in both places:
if not parsed.username:
return _reconstruct_url(parsed), None
# ... later ...
return _reconstruct_url(parsed), headers| if not (parsed.scheme in {"http", "https"} and parsed.netloc and parsed.hostname): | ||
| if ":" in url: | ||
| parsed_with_scheme = urlparse(f"http://{url}") | ||
| if parsed_with_scheme.netloc and parsed_with_scheme.hostname: | ||
| parsed = parsed_with_scheme | ||
| else: | ||
| return None, None | ||
| else: | ||
| return None, None |
Copilot
AI
Nov 24, 2025
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.
Incorrect handling of non-HTTP schemes: The condition at line 70 will incorrectly attempt to add http:// prefix to URLs with valid schemes other than http or https (e.g., ftp://example.com:21).
When the scheme is not in {"http", "https"}, the code falls through to line 71 and tries to prepend http://, resulting in http://ftp://example.com:21, which is invalid.
Consider revising the logic to:
if not parsed.scheme:
# No scheme provided - try adding http://
if parsed.netloc or ":" in url:
parsed_with_scheme = urlparse(f"http://{url}")
if parsed_with_scheme.netloc and parsed_with_scheme.hostname:
parsed = parsed_with_scheme
else:
return None, None
else:
return None, None
elif parsed.scheme not in {"http", "https"}:
# Unsupported scheme
return None, None
elif not (parsed.netloc and parsed.hostname):
# Missing netloc or hostname
return None, NoneThis approach:
- Checks for missing scheme separately
- Rejects unsupported schemes explicitly
- Validates netloc/hostname requirements
Fixes #33986.
Summary:
base_urlvalues (e.g.,ollama:11434) by defaulting tohttp://when the input resembleshost:port.Authorizationheaders whenuserinfocredentials are present, both for sync and async clients.Implementation details:
parse_url_with_authto accept scheme-less endpoints, producing a cleaned URL with explicit scheme and extracted auth headers.OllamaLLM,ChatOllama, orOllamaEmbeddings—they already consume the cleaned URL and headers.Why:
parse_url_with_authto return(None, None), leading Ollama clients to fall back to defaults and ignore the providedbase_url.Tests:
libs/partners/ollama/tests/unit_tests/test_auth.pyto cover the new cases.Notes:
httpto match common Ollama local deployments. Users can still explicitly providehttps://when appropriate.