-
Notifications
You must be signed in to change notification settings - Fork 553
fix(launcher): use the schema's draft_model global var, and validate global_vars keys #2232
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 | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -112,6 +112,52 @@ def _try_load_recipe(recipe_path: Path, source: Path) -> list[str]: | |||||||||||||||||||
| return [] | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| def _global_vars_schema() -> set[str] | None: | ||||||||||||||||||||
| """Field names accepted by ``GlobalVariables``, or None if it can't be read. | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Parsed out of ``core.py`` rather than imported: importing it pulls in ``nemo_run``, | ||||||||||||||||||||
| which is not a dependency of the pre-commit environment. | ||||||||||||||||||||
| """ | ||||||||||||||||||||
| core = _LAUNCHER_DIR / "core.py" | ||||||||||||||||||||
| try: | ||||||||||||||||||||
| source = core.read_text(encoding="utf-8") | ||||||||||||||||||||
| except OSError: | ||||||||||||||||||||
| return None | ||||||||||||||||||||
| match = re.search(r"^class GlobalVariables.*?(?=^@|\Z)", source, re.MULTILINE | re.DOTALL) | ||||||||||||||||||||
| if not match: | ||||||||||||||||||||
| return None | ||||||||||||||||||||
| return set(re.findall(r"^\s{4}(\w+)\s*:", match.group(0), re.MULTILINE)) | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| def _check_global_vars(pipeline: dict, path: Path) -> list[str]: | ||||||||||||||||||||
| """Reject ``global_vars`` keys the launcher's dataclass cannot accept. | ||||||||||||||||||||
|
|
||||||||||||||||||||
| ``global_vars`` is a fixed-field dataclass, not a free-form mapping, so an unknown key | ||||||||||||||||||||
| fails at launch with ``No parameter named 'X' exists`` — after the user has set up a | ||||||||||||||||||||
| cluster environment. This has now bitten twice (OMNIML-5024, then the Nemotron-3.5 | ||||||||||||||||||||
| DSpark warm-start example), so it is checked here instead. | ||||||||||||||||||||
| """ | ||||||||||||||||||||
| schema = _global_vars_schema() | ||||||||||||||||||||
| global_vars = pipeline.get("global_vars") | ||||||||||||||||||||
| if schema is None or not isinstance(global_vars, dict): | ||||||||||||||||||||
| return [] | ||||||||||||||||||||
|
Comment on lines
+140
to
+143
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. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Do not skip dangling-reference checks when If a task contains Proposed fix schema = _global_vars_schema()
global_vars = pipeline.get("global_vars")
-if schema is None or not isinstance(global_vars, dict):
+if schema is None:
return []
+defined = global_vars if isinstance(global_vars, dict) else {}
errors = [
...
- for key in global_vars
+ for key in defined
if key not in schema
]
...
- if ref not in global_vars
+ if ref not in defined📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||
| errors = [ | ||||||||||||||||||||
| f"{path}: global_vars key {key!r} is not a field of GlobalVariables " | ||||||||||||||||||||
| f"(valid: {', '.join(sorted(schema))})" | ||||||||||||||||||||
| for key in global_vars | ||||||||||||||||||||
| if key not in schema | ||||||||||||||||||||
| ] | ||||||||||||||||||||
| # A reference to a key that is never defined interpolates to the literal | ||||||||||||||||||||
| # ``<<global_vars.X>>`` and reaches the job as a nonsense path. | ||||||||||||||||||||
| refs = sorted(set(re.findall(r"<<global_vars\.(\w+)>>", path.read_text("utf-8")))) | ||||||||||||||||||||
| errors.extend( | ||||||||||||||||||||
| f"{path}: <<global_vars.{ref}>> is referenced but never defined" | ||||||||||||||||||||
| for ref in refs | ||||||||||||||||||||
| if ref not in global_vars | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| return errors | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| def _scan_launcher_yaml(path: Path) -> list[str]: | ||||||||||||||||||||
| errors: list[str] = [] | ||||||||||||||||||||
| try: | ||||||||||||||||||||
|
|
@@ -124,6 +170,8 @@ def _scan_launcher_yaml(path: Path) -> list[str]: | |||||||||||||||||||
| if not isinstance(pipeline, dict): | ||||||||||||||||||||
| return [] | ||||||||||||||||||||
|
|
||||||||||||||||||||
| errors.extend(_check_global_vars(pipeline, path)) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| for task in pipeline.values(): | ||||||||||||||||||||
| if not isinstance(task, dict): | ||||||||||||||||||||
| continue | ||||||||||||||||||||
|
|
||||||||||||||||||||
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.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Trigger a full scan when
core.pychanges._global_vars_schemamakestools/launcher/core.pyan input to this hook. However,_select_targetsonly scans all YAML files when this hook changes. If a later change removes or renames aGlobalVariablesfield, staging onlytools/launcher/core.pyleaves existing YAML files unchecked and can preserve launch-time failures. Treattools/launcher/core.pyas a full-scan trigger and add a regression test.🤖 Prompt for AI Agents