fix(platform-runner): seed NMP_BASE_URL from config platform.base_url - #706
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesBase URL Resolution
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
mckornfield
approved these changes
Jul 15, 2026
…e_url apply_run_environment derived NMP_BASE_URL from the bind host/port (a loopback address in standalone mode) and set it via setdefault. Because get_base_url() prefers the NMP_BASE_URL env var over the config file, an explicit platform.base_url in the config was silently shadowed by the runner-derived loopback value — making the setting a no-op. This broke container-mode agent deployments: the gateway URL injected into a deployed agent's NAT config (llms.*.base_url) resolved to 127.0.0.1, which inside the agent container is the container itself, so the agent's outbound model calls failed. Seed NMP_BASE_URL from the *host* of an explicit platform.base_url in the config file (read from the raw YAML so a file-set value is distinguishable from the schema default), paired with the port the server actually binds. Only the host is honored — a config that hardcodes a port (e.g. :8080) must not point internal in-process clients (and the embedded PDP) at that port when the platform is launched on a different one (which the e2e harness always does, and any 'nemo services run --port' other than 8080 does); using the config port there leaves internal HTTP clients unable to reach the server, so the platform never becomes ready. An externally-provided NMP_BASE_URL (Helm/k8s) still wins via setdefault, preserving deployed-mode behavior. Signed-off-by: Ben McCown <bmccown@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/nmp_platform_runner/tests/test_config.py (1)
225-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate
tmp_pathaspathlib.Path.Import
Pathnormally and type each changed fixture parameter astmp_path: Path.As per coding guidelines, “prefer concrete type hints over string-based type hints, and do not import those types only under
TYPE_CHECKING; import them normally when possible.”Also applies to: 230-230, 238-238, 248-248, 254-254, 260-260
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nmp_platform_runner/tests/test_config.py` at line 225, Import pathlib.Path normally in the test module and update each affected fixture parameter in _write_config and the other referenced test helpers to use the concrete annotation tmp_path: Path. Replace any string-based or missing tmp_path annotations while preserving the existing helper behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/nmp_platform_runner/src/nmp/platform_runner/config.py`:
- Around line 182-187: Update the base URL initialization around
_config_file_base_url_host to catch urlparse failures from malformed bracketed
IPv6 configuration values. On parse failure, fall back to the external
NMP_BASE_URL handling via env.setdefault instead of aborting startup, while
preserving the existing configured-host and url_host defaults for valid inputs.
In `@packages/nmp_platform_runner/tests/test_config.py`:
- Around line 230-252: The apply_run_environment URL construction currently
replaces configured schemes with http. Preserve the parsed scheme from the
configured platform base URL when generating NMP_BASE_URL and
NMP_AUTH_POLICY_DECISION_POINT_BASE_URL, including when applying the bind port;
add HTTPS coverage alongside the existing tests to verify https:// remains
unchanged.
---
Nitpick comments:
In `@packages/nmp_platform_runner/tests/test_config.py`:
- Line 225: Import pathlib.Path normally in the test module and update each
affected fixture parameter in _write_config and the other referenced test
helpers to use the concrete annotation tmp_path: Path. Replace any string-based
or missing tmp_path annotations while preserving the existing helper behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ab4841f5-1c43-489f-9dec-95655a832510
📒 Files selected for processing (2)
packages/nmp_platform_runner/src/nmp/platform_runner/config.pypackages/nmp_platform_runner/tests/test_config.py
tylersbray
reviewed
Jul 15, 2026
tylersbray
left a comment
tylersbray
left a comment
Contributor
There was a problem hiding this comment.
Non-blocking feedback from review — looks sufficient for the stated bug. Inline notes below are optional polish, not merge blockers.
Contributor
|
Non-blocking — documentation follow-up: |
tylersbray
approved these changes
Jul 15, 2026
tylersbray
left a comment
tylersbray
left a comment
Contributor
There was a problem hiding this comment.
+1 approval, left some optional non-blockers.
…d base_url Address PR review feedback on config-seeded NMP_BASE_URL: - Preserve the configured scheme: a config platform.base_url of https://... is no longer downgraded to http:// (parse and carry scheme + host). - Normalize the config-derived host through the same wildcard -> loopback translation as the bind host, so the bundled local.yaml default (http://0.0.0.0:8080) seeds a connectable http://127.0.0.1:<port> instead of the non-connectable wildcard, keeping internal PDP/readiness clients working. - Guard urlparse against malformed bracketed IPv6 (e.g. http://[::1): fail soft to the bind-derived default instead of aborting startup. - Correct the docstring to match behavior (scheme and host are honored). Tests: add HTTPS scheme preservation, malformed-IPv6 fallback, wildcard normalization, and a bundled default_config_path() regression guard; annotate tmp_path fixtures as pathlib.Path. Signed-off-by: Ben McCown <bmccown@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this touches
NMP_BASE_URL— the URL other components use to reach the platform's own API.The platform injects it into deployed agents so they can call back through the
Inference Gateway; if it's wrong for a containerized agent, that agent can't
reach any models. It can be set three ways, resolved in this order:
NMP_BASE_URLenv exists so a deployer (Helm/k8s) can point the platformat its real service address (e.g.
http://nemo-platform-api:8080); it takespriority because the deployer knows the true address.
platform.base_urlin config is the operator-facing equivalent for aself-hosted/standalone run — you'd set it to whatever address is reachable in
your environment (e.g. the docker bridge
http://172.17.0.1:8080socontainers can reach the host).
The bug
When you start the platform with
nemo services run, the runner(
apply_run_environment) auto-fillsNMP_BASE_URLfor standalone users whohaven't set it — it derives a value from the server's own bind address:
--host 0.0.0.0means "bind all interfaces," which isn't itself aconnectable address, so
_connect_host_for_internal_clientstranslates it tothe loopback
127.0.0.1(fine for in-process clients on the same host). Theresult —
http://127.0.0.1:8080— is written intoNMP_BASE_URLviasetdefault.The problem:
NMP_BASE_URL(env) ranks aboveplatform.base_url(config).So the runner's auto-derived loopback value silently overrides whatever you
set as
platform.base_urlin your config — the config setting becomes a no-op,with no error or warning. The only way to make it stick today is to also
export
NMP_BASE_URLyourself before launch, which defeats the purpose of theconfig field.
The user-visible symptom: an agent deployed in a container gets
llms.*.base_url = http://127.0.0.1:8080, which inside the container points atthe container itself, so its model calls fail. (Helm/k8s deployments are
unaffected — there
NMP_BASE_URLis set externally to the real, reachableaddress, so nothing is being shadowed.)
Fix
apply_run_environmentnow uses an explicitplatform.base_urlfrom the configfile as the default it seeds
NMP_BASE_URLwith (read from raw YAML so afile-set value is distinguishable from the schema default), and only falls back
to the bind-derived loopback when the config doesn't set one. An externally
provided
NMP_BASE_URLstill wins viasetdefault, so Helm/k8s is unchanged.New precedence: external env > config
platform.base_url> bind-derived default.Testing
test_config.py: seeds from config, external env stillwins, falls back when config omits
base_url, falls back when the file ismissing. Full runner suite green.
platform.base_urlset (noNMP_BASE_URLenv), a docker-mode agent now deploys and invokes through thegateway (previously failed).
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
NMP_BASE_URLis derived: existing environment values are preserved; otherwise the configuredplatform.base_urlsupplies the host while the bind port is applied.NMP_AUTH_POLICY_DECISION_POINT_BASE_URLmatches the final value.Tests