fix: ensure model_url is used from endpoint config by thushan · Pull Request #88 · thushan/olla

thushan · 2025-12-09T11:16:53Z

This may address #86 and where specifying model_url had no effect for llamacpp's profile.

Now if model_url is specified in the endpoint config, it will use that and fall back only if it's not specified.

      - url: "http://192.168.0.187:8010"
        name: "vllm-dev"
        type: "vllm"
        priority: 100
        model_url: "/v1/models"
        health_check_url: "/health"
        check_interval: 2s
        check_timeout: 1s

Summary by CodeRabbit

Release Notes

New Features
- Discovery URLs can now be customised per endpoint, with automatic fallback to profile defaults when not specified.
Tests
- Added test coverage for custom discovery URL overrides and fallback behaviour across multiple endpoint types.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

…e profile.

coderabbitai · 2025-12-09T11:17:11Z

Walkthrough

Discovery URL resolution in the HTTP client adapter now supports per-endpoint overrides via ModelURLString. When set on an endpoint, this value is used for discovery instead of the platform profile's default URL. If empty, it falls back to the profile-derived URL, maintaining backward compatibility.

Changes

Cohort / File(s)	Summary
Discovery URL Override Logic `internal/adapter/discovery/http_client.go`	Modified `discoverWithProfile` to check `endpoint.ModelURLString` first; uses this custom URL if set, otherwise falls back to `platformProfile.GetModelDiscoveryURL(endpoint.URLString)`. Enables non-standard deployments to supply custom discovery endpoints.
Test Coverage for URL Overrides `internal/adapter/discovery/http_client_test.go`	Added `TestModelURLOverride` and `TestModelURLFallbackToProfileDefault` to validate override and fallback behaviour. Introduced `createTestEndpointWithModelURL` helper, adjusted test server routing to serve only on specified paths, and extended coverage across multiple endpoint types (Ollama, vLLM, LlamaCpp, LM Studio, OpenAI Compatible).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Straightforward conditional logic addition with clear fallback semantics
Test coverage is well-structured and mirrors the implementation symmetry
Limited scope: changes confined to discovery URL resolution without affecting broader control flow

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarises the main change: adding support for using a custom model_url from endpoint configuration instead of always relying on profile defaults.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch hotfix/endpoint/model-url

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (3)

internal/adapter/discovery/http_client.go (1)

141-148: Discovery URL override logic matches intent and preserves fallback behaviour

Using endpoint.ModelURLString when non-empty and falling back to platformProfile.GetModelDiscoveryURL(endpoint.URLString) cleanly enables per-endpoint overrides while keeping existing behaviour untouched when no override is configured. The change is minimal, readable, and keeps error handling tied to the actual URL being called via NetworkError{URL: discoveryURL, ...}, which is useful for debugging.

The only thing to be mindful of is that ModelURLString should always be initialised as a fully-qualified URL (not just a path) at construction time; from the tests and helpers this looks to be the case, so no change is strictly required here.

internal/adapter/discovery/http_client_test.go (2)

625-727: Override tests correctly exercise per-endpoint ModelURLString behaviour

The table-driven TestModelURLOverride does a good job of proving that discovery honours an endpoint-level ModelURLString for each profile: the test server only succeeds on the custom path, so any regression back to profile defaults would surface as a 404/error. The requestedPath assertion is a nice guard to ensure the override path is actually used, not just “some” successful URL.

The profileDefaultPath field in the test cases isn’t referenced in the body; it mainly serves as documentation of what the defaults are. You could either remove it or add a small assertion (e.g. that requestedPath != profileDefaultPath) if you want it to play an active role, but that’s purely cosmetic.

729-810: Fallback-to-profile-default tests solidify backwards-compatible semantics

TestModelURLFallbackToProfileDefault clearly verifies that when ModelURLString is empty, discovery routes via the profile’s default path for each profile type. The server-side 404-on-anything-else pattern ensures you’d immediately see a regression in the default URL mapping, and the requestedPath assertion makes the behaviour explicit.

This nicely protects the original behaviour while allowing the new override semantics to coexist.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3d602ed and b47df2f.

📒 Files selected for processing (2)

internal/adapter/discovery/http_client.go (1 hunks)
internal/adapter/discovery/http_client_test.go (3 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

internal/adapter/discovery/http_client_test.go (3)

internal/core/domain/profile.go (5)

ProfileOllama (4-4)

ProfileVLLM (9-9)

ProfileLlamaCpp (6-6)

ProfileLmStudio (7-7)

ProfileOpenAICompatible (10-10)

internal/adapter/discovery/http_client.go (1)

NewHTTPModelDiscoveryClientWithDefaults (54-71)

internal/core/domain/endpoint.go (2)

Endpoint (19-40)

StatusHealthy (53-53)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

GitHub Check: Cross-Platform Build Validation (windows-latest)

🔇 Additional comments (1)

internal/adapter/discovery/http_client_test.go (1)

817-840: Test helpers correctly distinguish default vs override discovery URL semantics

Updating createTestEndpoint to initialise ModelUrl while leaving ModelURLString empty makes the default test endpoints behave like production ones that rely purely on profile-derived discovery URLs. The separate createTestEndpointWithModelURL helper then cleanly models the override case by setting both ModelUrl and ModelURLString only when an explicit override is provided.

This separation keeps the tests clear about when they’re exercising default behaviour vs override behaviour and aligns well with the new logic in discoverWithProfile.

Also applies to: 842-867

ensure that model_url is used from endpoint config and fallback is th…

b47df2f

…e profile.

coderabbitai bot reviewed Dec 9, 2025

View reviewed changes

thushan merged commit aa24bb6 into main Dec 9, 2025
6 checks passed

thushan deleted the hotfix/endpoint/model-url branch December 9, 2025 11:20

thushan mentioned this pull request Dec 10, 2025

fix: Alternative method of resolving profile paths #90

Merged

chenrui333 mentioned this pull request Dec 15, 2025

olla 0.0.22 chenrui333/homebrew-tap#3227

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: ensure model_url is used from endpoint config#88

fix: ensure model_url is used from endpoint config#88
thushan merged 1 commit intomainfrom
hotfix/endpoint/model-url

thushan commented Dec 9, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Dec 9, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

thushan commented Dec 9, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Release Notes

Uh oh!

coderabbitai bot commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

thushan commented Dec 9, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 9, 2025 •

edited

Loading