Skip to content

Commit 797db29

Browse files
authored
fix(config): aggregated error for unset '???' config values (#1575)
## What A required config value left unset (OmegaConf MISSING, `???`) currently surfaces as an opaque `omegaconf.errors.MissingMandatoryValue` — **one field at a time**, raised deep in parsing (while resolving inheritance), with no guidance on how to set it. This adds a fast-fail check in `parse()` that raises a single `ConfigMissingValuesError` listing **every** unset value with a ready-to-use override example. ### Before / after ``` # before omegaconf.errors.MissingMandatoryValue: Missing mandatory value: swe_agents.responses_api_agents.swe_agents.container_formatter # after 2 required config value(s) are unset (still '???') after merging: - swe_agents.responses_api_agents.swe_agents.container_formatter - swe_agents.responses_api_agents.swe_agents.dataset_path Provide each value via a CLI override, in env.yaml, or in a config you pass via config_paths. For example, on the command line: ++swe_agents.responses_api_agents.swe_agents.container_formatter=<value> ++swe_agents.responses_api_agents.swe_agents.dataset_path=<value> ``` ## How - New `ConfigMissingValuesError(ValueError)` in `config_types.py`. - `collect_missing_value_paths()` / `raise_on_missing_values()` in `global_config.py`. The scan runs **after** all sources are merged (CLI + env.yaml + config_paths) **and after** `_recursively_swap_keys` — so that the `_delete_key` / `_inherit_from` / `_copy` directives have been applied first. By that point any remaining `???` is genuinely unset (not a value that is about to be deleted, or moved/filled by a swap), so it's reported with no false positives. `_recursively_swap_keys` itself is made missing-tolerant (`items_ex(resolve=False)`) so a real `???` doesn't trip the opaque `MissingMandatoryValue` before the aggregated scan reports it. - The walk uses `OmegaConf.to_container(resolve=False, throw_on_missing=False)`, so it never raises on MISSING values or unresolved `${...}` interpolations. ## Scope / safety Base configs that intentionally ship `???` (api keys, container paths, model names — ~33 files) are unaffected: they're filled at run time via CLI/env, and no test parses a bare `???` config. This only changes the *error* a user sees when they forget to supply one. ## Testing - `test_collect_missing_value_paths` — nested dict + list, asserts `["a", "b.d", "e[1]"]`. - `test_get_global_config_dict_raises_on_missing_values` — asserts the dotted path and the `++...=<value>` hint appear in the message. - `pytest tests/unit_tests/test_global_config.py` 28/28; related `test_train_data_utils` / `test_config_types_help` / `test_rollout_collection` 54/54. ruff clean. No new dependencies. Part of #1205 (friction point 10). Companion to #1561 (friction 3). --------- Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>
1 parent 07dfc48 commit 797db29

4 files changed

Lines changed: 377 additions & 15 deletions

File tree

fern/versions/latest/pages/troubleshooting/configuration.mdx

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,19 @@ See the [Configuration reference](/reference/configuration) for complete configu
1616

1717
Errors that prevent servers from starting.
1818

19-
### Missing Mandatory Value
19+
### Unset Required Config Value(s)
2020

2121
```
22-
omegaconf.errors.MissingMandatoryValue: Missing mandatory value: policy_api_key
22+
nemo_gym.config_types.ConfigMissingValuesError: 1 required config value(s) are unset (still '???') after merging:
23+
- policy_api_key
24+
25+
Provide each value via a CLI override, in env.yaml, or in a config you pass via config_paths.
26+
For example, on the command line:
27+
++policy_api_key=<value>
2328
```
2429

25-
**When**: A required variable (usually in `env.yaml`) is not defined.
30+
**When**: One or more required variables (usually in `env.yaml`) are left unset (`???`) after all
31+
config sources are merged. Every unset value is listed together in this single error.
2632

2733
**Fix**: Add the missing value to `env.yaml`:
2834

@@ -39,11 +45,12 @@ ng_run "+config_paths=[config.yaml]" +policy_api_key=sk-your-api-key
3945
### Server Reference Not Found
4046

4147
```
42-
AssertionError: Could not find ResourcesServerRef(type='resources_servers', name='typo_weather')
43-
in the list of available servers: [ResourcesServerRef(...), ...]
48+
nemo_gym.config_types.ServerRefNotFoundError: In server instance 'my_agent', field 'resources_server' references resources_servers/'typo_weather', which is not defined in the merged config.
49+
Did you mean: 'weather'?
4450
```
4551

46-
**When**: A server config references another server that doesn't exist or isn't loaded.
52+
**When**: A server config references another server that doesn't exist or isn't loaded. The error
53+
names the referencing instance/field and, when a similarly named server exists, suggests it.
4754

4855
**Common causes**:
4956
- Typo in the server name

nemo_gym/config_types.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,10 @@ class ServerRefNotFoundError(ValueError):
140140
"""A server cross-reference points to an instance that is not defined in the merged config."""
141141

142142

143+
class ConfigMissingValuesError(ValueError):
144+
"""One or more required config values are still unset (OmegaConf '???') after merging."""
145+
146+
143147
########################################
144148
# Dataset configs for handling and upload/download
145149
########################################

nemo_gym/global_config.py

Lines changed: 96 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -28,14 +28,15 @@
2828
import rich
2929
import wandb
3030
import wandb.util
31-
from omegaconf import DictConfig, ListConfig, OmegaConf, open_dict
31+
from omegaconf import MISSING, DictConfig, ListConfig, OmegaConf, open_dict
3232
from openai import __version__ as openai_version
3333
from pydantic import BaseModel, ConfigDict, TypeAdapter, ValidationError
3434
from ray import __version__ as ray_version
3535
from wandb import Run
3636

3737
from nemo_gym import CACHE_DIR, PARENT_DIR, RESULTS_DIR, WORKING_DIR
3838
from nemo_gym.config_types import (
39+
ConfigMissingValuesError,
3940
ServerInstanceConfig,
4041
ServerRefNotFoundError,
4142
WANDBConfig,
@@ -69,6 +70,11 @@
6970
INHERIT_FROM_KEY_NAME = "_inherit_from"
7071
COPY_KEY_NAME = "_copy"
7172
DELETE_KEY_KEY_NAME = "_delete_key"
73+
74+
# Sentinel returned by _recursive_index_dict_using_path when a referenced swap/copy/inherit path is
75+
# unset (a '???' leaf or ancestor). Distinct object so callers can branch on it without mistaking a
76+
# real config value (e.g. a literal "???" or a DictConfig) for "missing".
77+
_MISSING_REF = object()
7278
NEMO_GYM_LOG_DIR_KEY_NAME = "nemo_gym_log_dir"
7379
NEMO_GYM_RESERVED_TOP_LEVEL_KEYS = [
7480
CONFIG_PATHS_KEY_NAME,
@@ -291,6 +297,55 @@ def validate_and_populate_defaults(
291297

292298
return disallowed_ports
293299

300+
def collect_missing_value_paths(self, config: DictConfig) -> List[str]:
301+
"""Return the dotted paths of every unset (OmegaConf '???') leaf, without raising.
302+
303+
We convert to a plain container with `resolve=False, throw_on_missing=False` so that
304+
neither MISSING values nor unresolved interpolations (`${...}`) cause an exception — then
305+
walk the plain structure. Iterating or indexing the live DictConfig would raise.
306+
"""
307+
container = OmegaConf.to_container(config, resolve=False, throw_on_missing=False)
308+
return self._walk_missing_value_paths(container)
309+
310+
def _walk_missing_value_paths(self, node, prefix: str = "") -> List[str]:
311+
missing_paths: List[str] = []
312+
if isinstance(node, dict):
313+
for key, value in node.items():
314+
path = f"{prefix}.{key}" if prefix else str(key)
315+
if value == MISSING:
316+
missing_paths.append(path)
317+
else:
318+
missing_paths.extend(self._walk_missing_value_paths(value, path))
319+
elif isinstance(node, list):
320+
for i, value in enumerate(node):
321+
path = f"{prefix}[{i}]"
322+
if value == MISSING:
323+
missing_paths.append(path)
324+
else:
325+
missing_paths.extend(self._walk_missing_value_paths(value, path))
326+
return missing_paths
327+
328+
def raise_on_missing_values(self, global_config_dict: DictConfig) -> None:
329+
"""Fail fast with one actionable error listing every unset '???' value.
330+
331+
Without this, the first unset value surfaces deep in the run pipeline as an opaque
332+
omegaconf MissingMandatoryValue, one field at a time and with no override guidance.
333+
"""
334+
missing_paths = self.collect_missing_value_paths(global_config_dict)
335+
if not missing_paths:
336+
return
337+
338+
missing_list = "\n".join(f" - {p}" for p in missing_paths)
339+
override_examples = "\n".join(f" ++{p}=<value>" for p in missing_paths[:3])
340+
raise ConfigMissingValuesError(
341+
f"""{len(missing_paths)} required config value(s) are unset (still '???') after merging:
342+
{missing_list}
343+
344+
Provide each value via a CLI override, in env.yaml, or in a config you pass via config_paths.
345+
For example, on the command line:
346+
{override_examples}"""
347+
)
348+
294349
def _recursively_hide_secrets(self, dict_config: DictConfig) -> None:
295350
with open_dict(dict_config):
296351
self._recursively_hide_secrets_helper(dict_config)
@@ -318,7 +373,11 @@ def _recursively_swap_keys(self, dict_config: DictConfig) -> None:
318373
def _recursively_swap_keys_helper(
319374
self, dict_config: DictConfig, original_dict_config: DictConfig, frozen_dict_config: DictConfig
320375
) -> None:
321-
for k, v in list(dict_config.items()):
376+
# items_ex(resolve=False) yields raw values: directive strings like "${inherit_from:...}"
377+
# come back unresolved (so the swap detection below still matches), and a missing ('???')
378+
# leaf is returned as-is instead of raising MissingMandatoryValue mid-swap. Any genuinely
379+
# unset values are reported together by raise_on_missing_values, which runs after this pass.
380+
for k, v in list(dict_config.items_ex(resolve=False)):
322381
is_delete_property = isinstance(v, DictConfig) and DELETE_KEY_KEY_NAME in v
323382

324383
if is_delete_property:
@@ -335,7 +394,12 @@ def _recursively_swap_keys_helper(
335394
if isinstance(v, (DictConfig, dict)):
336395
self._recursively_swap_keys_helper(v, original_dict_config, frozen_dict_config)
337396
elif isinstance(v, (ListConfig, list)):
338-
for inner_v in v:
397+
# Iterate without resolving so a missing ('???') element doesn't raise mid-swap (it's
398+
# a scalar, so it's skipped here and reported later by raise_on_missing_values).
399+
for i in range(len(v)):
400+
if isinstance(v, ListConfig) and OmegaConf.is_missing(v, i):
401+
continue
402+
inner_v = v[i]
339403
if isinstance(inner_v, (DictConfig, dict)):
340404
self._recursively_swap_keys_helper(inner_v, original_dict_config, frozen_dict_config)
341405

@@ -362,16 +426,24 @@ def _recursively_swap_keys_helper(
362426

363427
path_to_swap = path_to_swap.split(".")
364428

365-
# Pop the swapped value
429+
# Pop the swapped value. A '???' leaf or ancestor on the source path comes back as the
430+
# _MISSING_REF sentinel (not a dict/str), so guard the .pop()/.merge() that would crash on it.
366431
dict_containing_key_to_swap = self._recursive_index_dict_using_path(
367432
original_dict_config, path_to_swap[:-1]
368433
)
369-
if is_swap:
434+
if is_swap and dict_containing_key_to_swap is not _MISSING_REF:
370435
# Pop with a default since multiple configs may refer to the same path
371436
# We don't want to pop if it's just a copy
372437
dict_containing_key_to_swap.pop(path_to_swap[-1], None)
373438

374439
swapped_value = self._recursive_index_dict_using_path(frozen_dict_config, path_to_swap)
440+
441+
# If the source (leaf or an ancestor) is unset, the target inherits MISSING; skip the
442+
# property-merge and _delete_key handling and let raise_on_missing_values report it.
443+
if dict_containing_key_to_swap is _MISSING_REF or swapped_value is _MISSING_REF:
444+
dict_config[k] = "???"
445+
continue
446+
375447
if is_swap_property or is_copy_property:
376448
swapped_value = OmegaConf.merge(swapped_value, v)
377449

@@ -384,11 +456,22 @@ def _recursively_swap_keys_helper(
384456
for key in keys_to_delete:
385457
dict_config[k].pop(key)
386458

387-
def _recursive_index_dict_using_path(self, dict_config: DictConfig, path: List[str]) -> DictConfig:
459+
def _recursive_index_dict_using_path(self, dict_config: DictConfig, path: List[str]) -> "DictConfig | object":
388460
for k in path:
389-
if k not in dict_config:
461+
# Use _get_node so a referenced value that is unset ('???') can be detected without
462+
# resolving it (indexing a MISSING leaf would raise an opaque error). A genuinely
463+
# absent key still errors clearly.
464+
node = dict_config._get_node(k) if isinstance(dict_config, DictConfig) else None
465+
if node is None:
390466
raise ValueError(f"Path specified does not exist in config: {path}")
391467

468+
# The referenced value (or an ancestor of it) is unset. Return the _MISSING_REF sentinel
469+
# so the caller makes the swap/copy/inherit target '???' too (instead of calling .pop()/
470+
# OmegaConf.merge() on a bare string and crashing); raise_on_missing_values then reports
471+
# it with an actionable message instead of an opaque interpolation error.
472+
if OmegaConf.is_missing(dict_config, k):
473+
return _MISSING_REF
474+
392475
dict_config = dict_config[k]
393476

394477
return dict_config
@@ -438,6 +521,12 @@ def parse(self, parse_config: Optional[GlobalConfigDictParserConfig] = None) ->
438521

439522
self._recursively_swap_keys(global_config_dict)
440523

524+
# Fail fast with one actionable error if any required value is still '???'. Runs *after*
525+
# _recursively_swap_keys so that _delete_key/_inherit_from/_copy have been applied first —
526+
# a '???' in a deleted or overwritten branch is not reported. Otherwise the first unset
527+
# value surfaces as an opaque MissingMandatoryValue deep in the pipeline.
528+
self.raise_on_missing_values(global_config_dict)
529+
441530
# TODO @bxyu-nvidia: We need a better way of handling dummy model configs
442531
with open_dict(global_config_dict):
443532
for top_level_value in global_config_dict.values():

0 commit comments

Comments
 (0)