Skip to content

fix(cli): report unresolved config interpolations cleanly in manifest validation - #2904

Open
AmirF194 wants to merge 2 commits into
NVIDIA-NeMo:mainfrom
AmirF194:fix/2224-manifest-validate-interpolation-traceback
Open

fix(cli): report unresolved config interpolations cleanly in manifest validation#2904
AmirF194 wants to merge 2 commits into
NVIDIA-NeMo:mainfrom
AmirF194:fix/2224-manifest-validate-interpolation-traceback

Conversation

@AmirF194

@AmirF194 AmirF194 commented Aug 31, 2026

Copy link
Copy Markdown

gym env validate promises a clean message and no traceback for a bad config, but that only
holds for the legacy config path. _resolve_manifest_composition (the manifest-backed
workload path validate_environment uses) calls GlobalConfigDictParser.parse() directly,
without the InterpolationResolutionError -> ConfigInterpolationError translation that
set_global_config_dict applies at the CLI's other config-loading boundary (#2310). So a
manifest referencing an unresolved ${key} interpolation raises the raw omegaconf exception
straight through @exit_cleanly_on_config_error, which only catches ConfigError, and the
user sees a full stack trace instead of the actionable message the CLI ships everywhere else.

Extracted the translation from set_global_config_dict into a shared
translate_interpolation_error helper in nemo_gym/global_config.py and applied it at the
manifest-validation boundary too, so both callers raise the same ConfigInterpolationError
with the same actionable message.

Verified in a clean container (python 3.13, uv sync --extra dev --extra telemetry):

  • New test test_unresolved_interpolation_is_reported_cleanly_not_as_a_traceback fails on
    main with a raw omegaconf.errors.InterpolationKeyError and passes on this branch.
  • tests/unit_tests/test_environment_validation.py and tests/unit_tests/test_global_config.py
    pass in full (152/152).
  • ruff check and ruff format --diff are clean on the changed files.
  • Not run: the full CI suite (server suite, coverage gate); only the two unit test modules
    touched by this change.

… validation

gym env validate promises a clean message and no traceback for a bad config,
but that only held for the legacy config path. The manifest-backed workload
path (_resolve_manifest_composition) called GlobalConfigDictParser.parse()
directly, without the InterpolationResolutionError -> ConfigInterpolationError
translation set_global_config_dict applies at the CLI's other config-loading
boundary (NVIDIA-NeMo#2310). An unresolved ${key} interpolation in a manifest config
raised the raw omegaconf exception straight through
@exit_cleanly_on_config_error, which only catches ConfigError.

Extract the translation into a shared translate_interpolation_error helper
and apply it at the manifest-validation boundary too.

Fixes NVIDIA-NeMo#2224

Signed-off-by: Amir Fathi <amirfathi.me@gmail.com>
@copy-pr-bot

copy-pr-bot Bot commented Aug 31, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@nemo-automation-bot nemo-automation-bot Bot added the community-request Issue reported or requested by someone from the community label Aug 31, 2026
@ananthsub
ananthsub requested review from Glorf and marta-sd September 1, 2026 05:34
@marta-sd

marta-sd commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

/claude review

@claude

claude Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

SHIP — LGTM, no reliability concerns.

Pure refactor: the interpolation-error translation block is extracted verbatim from set_global_config_dict into a module-level translate_interpolation_error helper (returns the exception; callers raise ... from e, so cause-chaining is preserved). It's then applied around parser.parse(...) in _resolve_manifest_composition, so validate_environment surfaces the same clean, actionable ConfigInterpolationError instead of a raw omegaconf traceback.

Checks:

  • Logic (regex, full_key, message) is byte-identical to the original — no behavior change on the existing path.
  • Imports resolve: re, InterpolationResolutionError, ConfigInterpolationError present in both modules; helper exported and imported into validation.py.
  • No async, verifier/scoring, or public-API surface touched.
  • New test drives the real path (num_repeats: ${undefined_key}) and asserts on exception type + message, not vacuous.

Nit (NOTE, author's call): the try in _resolve_manifest_composition wraps only parser.parse, not filter_for_server_instance_configs — correct, since interpolation resolves during parse.

@marta-sd marta-sd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution @AmirF194 ! I've edited the description slightly - the issue #2224 is related to legacy path, not the manifest validation, so I dropped the "Fixes" part to keep the issue open.

manifest_path = _asset(tmp_path)
config_path = manifest_path.with_name("config.yaml")
config_path.write_text(
config_path.read_text(encoding="utf-8").replace(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test doesn't check the manifest path, but the legacy path. Please test your code with incomplete manifest instead.

)
)
)
except InterpolationResolutionError as e:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the only place where we had this raw interpolation error? If not, maybe it's worth updating all at once

@marta-sd

marta-sd commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

/ok to test a74ea0e

@svcnvidia-nemo-ci svcnvidia-nemo-ci added the waiting-on-customer Waiting on the original author to respond label Sep 1, 2026
@github-actions github-actions Bot added the sla:review-overdue Review response is over the one-business-day SLA label Sep 2, 2026
@AmirF194

AmirF194 commented Sep 8, 2026

Copy link
Copy Markdown
Author

This is a pure extraction: the interpolation-error translation moved out of set_global_config_dict unchanged, nothing else touched. claude's automated review called it clean a week ago and Glorf hasn't weighed in since, so flagging it in case it fell off the queue.

@svcnvidia-nemo-ci svcnvidia-nemo-ci added waiting-on-maintainers Waiting on maintainers to respond and removed waiting-on-customer Waiting on the original author to respond labels Sep 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-request Issue reported or requested by someone from the community sla:review-overdue Review response is over the one-business-day SLA waiting-on-maintainers Waiting on maintainers to respond

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants