-
Notifications
You must be signed in to change notification settings - Fork 345
fix(cli): report unresolved config interpolations cleanly in manifest validation #2904
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: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |
|
|
||
| import pytest | ||
|
|
||
| from nemo_gym.config_types import ConfigInterpolationError | ||
| from nemo_gym.environment.manifest import EnvironmentManifest, dump_manifest, load_manifest | ||
| from nemo_gym.environment.validation import ( | ||
| EnvironmentValidationError, | ||
|
|
@@ -357,6 +358,21 @@ def test_benchmark_uses_root_prompt_without_executing_prepare(tmp_path: Path) -> | |
| assert report.datasets[0].prompt_config.endswith("prompts/default.yaml") | ||
|
|
||
|
|
||
| def test_unresolved_interpolation_is_reported_cleanly_not_as_a_traceback(tmp_path: Path) -> None: | ||
| 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( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Checked with a debug trace: this goes through the new except in Not sure what distinguishes "manifest path" from "legacy path" here in your read, since the manifest branch is the one that was actually broken. Happy to change the test if you can point at the specific gap. |
||
| "jsonl_fpath: environments/demo/data/example.jsonl", | ||
| "jsonl_fpath: environments/demo/data/example.jsonl\n num_repeats: ${undefined_key}", | ||
| ), | ||
| encoding="utf-8", | ||
| ) | ||
|
|
||
| with pytest.raises(ConfigInterpolationError, match="undefined_key"): | ||
| validate_environment(manifest_path) | ||
|
|
||
|
|
||
| def test_malformed_benchmark_prompt_is_an_actionable_validation_error(tmp_path: Path) -> None: | ||
| manifest_path = _asset(tmp_path, kind="benchmark") | ||
| manifest_path.parent.joinpath("prompts/default.yaml").write_text("user: [broken\n", encoding="utf-8") | ||
|
|
||
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.
Is this the only place where we had this raw interpolation error? If not, maybe it's worth updating all at once
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.
Checked the other two direct
GlobalConfigDictParser().parse()call sites.benchmarks.py's strict branch is unguarded, but nothing calls it withstrict=Truein production, the one caller passesstrict=False, which already tolerates unresolved interpolations via_parse_no_environment_tolerating_unset_values._run_manifest_verifierin cli/env.py also calls.parse()unguarded, but its input is the fixedNO_MODEL_GLOBAL_CONFIG_DICTconstant with noconfig_paths, so there's nothing there that could reference an undefined key.Neither looked live to me, so I left them alone.