[Fix] urljoin fix#317
Conversation
Signed-off-by: Li, Tianmu <tianmu.li@intel.com>
Signed-off-by: Li, Tianmu <tianmu.li@intel.com>
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
There was a problem hiding this comment.
Code Review
This pull request updates the URL construction logic to ensure consistent behavior when joining base URLs with API routes. Specifically, it removes leading slashes from default routes in the APIType enum and ensures base URLs are suffixed with a slash before calling urljoin to prevent the loss of path components. A review comment suggests abstracting the repeated URL formatting logic into a shared helper function to improve maintainability and reduce duplication across the main code and test files.
There was a problem hiding this comment.
Pull request overview
Fixes incorrect URL construction when joining an endpoint base URL that includes a path component (e.g., https://openrouter.ai/api) with an API route, avoiding urljoin() behavior that can discard the base path.
Changes:
- Changed
APIType.default_route()values to be relative (no leading/) sourljoin()won’t discard the base URL path. - Normalized base endpoint URLs to always end with
/beforeurljoin()in the benchmark execution path and in integration tests. - Updated unit tests to match the new
default_route()contract.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/inference_endpoint/core/types.py |
Updates APIType.default_route() return values to relative routes (no leading /). |
src/inference_endpoint/commands/benchmark/execute.py |
Normalizes endpoint base URLs before joining with default_route(). |
tests/unit/config/test_schema.py |
Updates expected default routes to match the new relative-route contract. |
tests/unit/videogen/test_registration.py |
Updates expected VIDEOGEN default route. |
tests/unit/videogen/test_adapter.py |
Updates expected VIDEOGEN default route. |
tests/integration/test_multi_turn.py |
Ensures base URL normalization before urljoin() for integration requests. |
tests/integration/test_end_to_end_oracle.py |
Ensures base URL normalization before urljoin() for integration requests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Li, Tianmu <tianmu.li@intel.com>
viraatc
left a comment
There was a problem hiding this comment.
lgtm, thanks for the fix!
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
src/inference_endpoint/commands/probe.py:113
- With the new
urljoin(e.rstrip("/") + "/", api_type.default_route())behavior, providing an endpoint that already includes the API route (e.g.http://host:8000/v1/chat/completions) will now produce a duplicated path (.../v1/chat/completions/v1/chat/completions). If you intend--endpointsto always be a base URL/prefix, consider validating/rejecting endpoints whose path already ends withapi_type.default_route(), or detect that case and skip the join.
http_config = HTTPClientConfig(
endpoint_urls=[
urljoin(e.rstrip("/") + "/", api_type.default_route())
for e in endpoints.split(",")
],
src/inference_endpoint/commands/benchmark/execute.py:648
- This change fixes URL composition for endpoints that include a path prefix (e.g.
https://host/api), but there is no test that asserts the prefix is preserved (regression coverage for the originalurljoin(base, "/v1/..." )bug). Adding a unit/integration test that constructs an endpoint with a non-root base path and verifies the final endpoint URL includes that prefix would help prevent regressions.
http_config = config.settings.client.with_updates(
endpoint_urls=[
urljoin(e.rstrip("/") + "/", api_type.default_route())
for e in endpoints
],
|
Thanks for this. Can you please add a test to document the failure. |
Review Council — Multi-AI Code ReviewReviewed by: Claude (Codex unavailable — bwrap sandbox not supported on this kernel) | Depth: quick (diff < 200 lines) No issues found. Checked for critical bugs, crashes, security vulnerabilities, and data loss risks. 🤖 Generated with Claude Code |
What does this PR do?
urljoin with absolute path (leading "/" like "/v1/chat/completions") discards entire path of the base URL. E.g.: https://openrouter.ai/api produces https://openrouter.ai/v1/chat/completions instead of https://openrouter.ai/api/v1/chat/completions.
Type of change
Related issues
Testing
Checklist