Bundle the ScriptAutomation engine (migrate automation/ from mlperf-automations)#271
Bundle the ScriptAutomation engine (migrate automation/ from mlperf-automations)#271anandhu-eng wants to merge 7 commits into
Conversation
Moves the automation/ engine (ScriptAutomation: module.py, cache_utils.py, docker.py, apptainer.py, etc. — previously ~12,000 lines dynamically loaded from a cloned copy of mlcommons@mlperf-automations) directly into this repo at automation/, shipped as part of the mlcflow package. - mlc/action.py: new bundled_automation_path() resolves automation/<target> relative to the installed mlc package; find_target_folder() prefers it, falling back to scanning registered repos only if the bundled path is absent (preserves the dev-override escape hatch). - pyproject.toml: ship automation/ and automation/script/ as packages, including their non-.py data files (meta.json, README.md). - AGENTS.md / .claude/skill.md: updated to describe the engine as bundled. - tests/test_automation_bundled.py: covers the bundled-path resolution, dynamic import of ScriptAutomation, and the registered-repo fallback path. mlcommons@mlperf-automations now holds only script content; its automation/ folder has been removed there in a matching change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
🤖 AI PR Review Summary\n\nThis PR migrates the ScriptAutomation engine from the external
|
The removal decision is still pending; mlperf-automations has not had automation/ deleted. mlcflow's bundled copy is authoritative and preferred by find_target_folder(), but the old copy stays in place until a separate decision to remove it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
🤖 AI PR Review Summary\n\nThis PR migrates the ScriptAutomation engine from the external mlperf-automations repo into the current mlcflow repo under the automation/ directory, making mlcflow both the CLI driver and the script execution engine. The script content remains in mlperf-automations and is pulled on demand. The find_target_folder() logic is updated to prefer the bundled automation path in mlcflow, falling back to external repos only as a last resort. Documentation and developer guidance are extensively updated to reflect this architectural change. Risks include ensuring backward compatibility with existing workflows and avoiding circular imports between mlc and automation modules. Design-wise, the change centralizes engine code, simplifying development but requires careful handling of dynamic imports and repo fallback logic. |
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
🤖 AI PR Review Summary\n\nThis PR migrates the script execution engine (ScriptAutomation) from the external mlperf-automations repo into the current mlcflow repo under the automation/ directory. The engine is now bundled and loaded primarily from this repo, with fallback to external repos only as a last resort. Documentation and code comments are updated extensively to clarify this new architecture. This reduces cross-repo dependencies and simplifies development but introduces some risk around ensuring backward compatibility and maintaining the fallback logic. The design maintains a one-way import direction from automation/ to mlc/, avoiding circular imports. The PR also adds a large README for the automation/script/ folder explaining the engine's internal workings. |
… presence
find_target_folder("script") now always succeeds via the bundled engine,
so its absence can no longer signal that mlcommons@mlperf-automations
(the script content repo) needs pulling. CI's "Test automatic repo pull"
(mlc rm repo mlcommons@mlperf-automations -f && mlcr detect,cpu) exposed
this: after removing the repo, script_path was still found (bundled), so
the auto-pull never fired, and the script search failed with "no scripts
were found with tags: ['detect', 'cpu']".
Adds _content_repo_registered() to check directly whether
mlcommons@mlperf-automations is registered, independent of engine location,
and triggers the existing auto-pull path on that instead.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
🤖 AI PR Review Summary\n\nThis PR migrates the ScriptAutomation engine from the external mlperf-automations repo into the current mlcflow repo, bundling it directly under automation/script/module.py. The design shifts from dynamically loading the engine from an external repo to preferring the bundled engine path, with fallback to external repos only as a last resort. Script content remains in mlperf-automations and is pulled as before. The PR updates documentation and code comments extensively to clarify this new architecture and usage. Risks include ensuring backward compatibility with existing workflows, avoiding circular imports between mlc and automation packages, and maintaining the fallback mechanism for external overrides. The design improves developer experience by enabling direct edits to the engine within mlcflow, but requires careful management of import paths and repo scanning logic. |
_content_repo_registered() was checking a specific repo alias, which breaks for forks/custom clones (e.g. gateoverflow@mlperf-automations). Check generically instead: does any registered repo have a non-empty top-level script/ directory? Mirrors how find_target_folder() itself walks self.repos looking for automation/script/, just one level up. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| # folder, just one level up at <repo>/script/ instead of | ||
| # <repo>/automation/script/. | ||
| for repo in self.repos: | ||
| script_dir = os.path.join(repo.path, 'script') |
There was a problem hiding this comment.
Its a bit rudimentary logic. But ig its fine for now?
|
@copilot review the changes |
🤖 AI PR Review SummaryThis PR migrates the ScriptAutomation engine from the external mlperf-automations repo into the current mlcflow repo under automation/script/module.py, making mlcflow both the CLI driver and the primary script execution engine. The design now prefers the bundled automation path inside mlcflow and falls back to external repos only as a last resort. Documentation and code comments are updated extensively to clarify this new architecture and usage. Risks include potential confusion during transition due to coexistence of two automation copies, and the need to maintain backward compatibility with existing script content in mlperf-automations. The one-way import coupling and dynamic import mechanism are preserved to avoid circular dependencies. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -0,0 +1,926 @@ | |||
| # MLC Script Automation — How It Works | |||
There was a problem hiding this comment.
Add a comprehensive new README documenting the internal workings of the MLC Script Automation engine, improving developer understanding.
There was a problem hiding this comment.
Not an actionable suggestion — this is automation/script/README.md itself, the original documentation file copied over verbatim as part of the automation/ migration in commit 3e962eb. No further change needed.
| `mlperf-automations` repo and is pulled/cloned on demand — only the engine | ||
| that runs that content moved here. This file covers everything an AI agent | ||
| needs to contribute correctly to this repo. | ||
|
|
There was a problem hiding this comment.
The updated explanation of the two repos' roles is clearer and more accurate; good to emphasize that only the engine moved, not the script content.
| # walks registered repos looking for automation/script/ | ||
| # prefers the bundled automation/script/ shipped in this | ||
| # repo (bundled_automation_path); falls back to scanning | ||
| # registered repos for a custom/external override |
There was a problem hiding this comment.
The detailed explanation of the fallback logic for find_target_folder() is very helpful; consider adding a diagram or flowchart in future docs for visual clarity.
| directories) — those are pulled from `mlperf-automations` into `~/MLC/repos/` | ||
| like before. | ||
|
|
||
| --- |
There was a problem hiding this comment.
The note about the one-way import dependency from automation/ to mlc/ is critical to avoid circular imports; consider adding automated tests or lint rules to enforce this.
| cache_utils.py, docker.py, docker_utils.py, apptainer.py, doc.py, | ||
| experiment.py, help.py, lint.py, meta_schema.py, remote_run.py, | ||
| script_utils.py, validate.py | ||
| tests/ |
There was a problem hiding this comment.
The updated file layout summary is useful; consider adding a note about how to update or sync the mlperf-automations repo content if needed.
| @@ -0,0 +1,926 @@ | |||
| # MLC Script Automation — How It Works | |||
There was a problem hiding this comment.
This large new README is a great addition for documenting the script automation internals; ensure it is kept up to date with code changes and consider splitting into smaller files if it grows further.
f569ec8 to
f6b4d31
Compare
🤖 AI PR Review SummaryThis PR migrates the ScriptAutomation engine from the external mlperf-automations repo into the mlcflow repo, bundling the script execution engine directly within mlcflow under automation/script/module.py. The design change prioritizes loading the bundled engine first, falling back to external repos only as a last resort. The PR updates documentation extensively to clarify the new repo roles, loading logic, and contributor guidance. Risks include potential confusion during transition due to dual copies of the engine, and ensuring backward compatibility with existing script content still residing in mlperf-automations. The design improves maintainability by co-locating engine and CLI driver code but requires careful import management to avoid circular dependencies. |
| → call_script_module_function("run", run_args) | ||
| → find_target_folder("script") # walks registered repos | ||
| → find_target_folder("script") # bundled automation/script/ first, | ||
| # falls back to scanning registered repos |
There was a problem hiding this comment.
Good clarification that find_target_folder now prefers the bundled automation/script/ path first, then falls back to registered repos. This is a key design change.
| → ScriptAutomation(self, path).run(run_args) # engine in mlperf-automations | ||
| → ScriptAutomation(self, path).run(run_args) # engine bundled in this repo | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Updated comment clarifies that ScriptAutomation engine is now bundled in this repo, not loaded from mlperf-automations. This is important for understanding runtime behavior.
| `automation/script/module.py` in this repo (originally developed in | ||
| `mlperf-automations`, and now bundled here as the primary copy mlcflow | ||
| actually loads). `mlperf-automations` still has its own `automation/` folder | ||
| too — it was **not** removed there, kept for backward compatibility — but |
There was a problem hiding this comment.
The note about mlperf-automations still containing its own automation folder for backward compatibility is important to avoid confusion about multiple copies.
| `mlc`, `mlcr`, `mlcd`, and related commands, and it *bundles* the script | ||
| execution engine (`ScriptAutomation`, in `automation/`) directly in this repo. | ||
| Benchmark **content** (377+ script directories) still lives in the separate | ||
| `mlperf-automations` repo and is pulled/cloned on demand — only the engine |
There was a problem hiding this comment.
The updated description correctly states that the script execution engine is bundled in this repo, while script content remains in mlperf-automations. This distinction is critical for contributors.
| → find_target_folder("script") | ||
| # walks registered repos looking for automation/script/ | ||
| # prefers the bundled automation/script/ shipped in this | ||
| # repo (bundled_automation_path); falls back to scanning |
There was a problem hiding this comment.
The detailed explanation of how find_target_folder now prefers the bundled automation/script/ path and falls back to registered repos is very helpful for understanding the new loading order.
|
|
||
| mlcflow does **not** contain or execute benchmark logic. It finds the engine, | ||
| loads it, and hands off `run_args`. | ||
| - `mlcflow` (this repo) — the CLI driver *and* the script execution engine: |
There was a problem hiding this comment.
The explanation of dynamic_import_module prepending automation/ to sys.path to resolve relative imports is a useful implementation detail.
| `run_args`. It still does not contain benchmark *content* (individual script | ||
| directories) — those are pulled from `mlperf-automations` into `~/MLC/repos/` | ||
| like before. | ||
|
|
There was a problem hiding this comment.
Clear guidance for contributors that ScriptAutomation now lives in this repo and can be edited directly is a major usability improvement.
| mlc/ | ||
| main.py # CLI entry point; all short commands (mlcr, mlcd…) defined here | ||
| action.py # Base Action class: repo loading, index access, CRUD | ||
| action.py # Base Action class: repo loading, index access, CRUD, |
There was a problem hiding this comment.
The warning about avoiding circular imports from automation/ back into mlc/ at module load time is important to maintain the intended one-way dependency.
| cache_utils.py, docker.py, docker_utils.py, apptainer.py, doc.py, | ||
| experiment.py, help.py, lint.py, meta_schema.py, remote_run.py, | ||
| script_utils.py, validate.py | ||
| tests/ |
There was a problem hiding this comment.
The note about auto-pull using --branch=dev as a last resort fallback is a good clarification to reduce confusion about when auto-pull triggers.
| 2. **Auto-pull if missing**: if no `automation/script/` is found, it pulls | ||
| `mlcommons@mlperf-automations --branch=dev` automatically and retries once. | ||
| `Action` base. This now calls `bundled_automation_path("script")` first, | ||
| which resolves `automation/script/` relative to the installed `mlc` |
There was a problem hiding this comment.
The note about adding new function names in both call_script_module_function and ScriptAutomation is a good reminder to keep dispatch logic in sync.
| the `mlc/` package directory — works identically in an editable checkout | ||
| and in site-packages). Only if that's missing does it fall back to | ||
| scanning `self.repos` for a repo with its own `automation/script/` | ||
| directory (a dev-override escape hatch; not the normal path anymore). |
There was a problem hiding this comment.
The explicit mention of the one-way import direction from automation/ to mlc/ and avoidance of circular imports is a critical architectural note.
…pies mlperf-automations' automation/ copy is dead code (never executed, the bundled copy here always wins) - worth calling out explicitly so a contributor doesn't waste time editing the wrong copy expecting it to have an effect. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
🤖 AI PR Review SummaryThis PR documents and clarifies the migration of the ScriptAutomation engine from the external mlperf-automations repo into the mlcflow repo, making mlcflow both the CLI driver and the primary script execution engine host. It updates documentation to reflect that the engine code now lives in mlcflow's automation/ folder, while the script content remains in mlperf-automations. The change reduces cross-repo dependencies but introduces a version drift risk since the old automation/ folder in mlperf-automations remains but is no longer used. The PR also adds a new automation/ directory with the engine code and a large README.md explaining the script automation internals. Risks include potential confusion about which automation/ copy is authoritative and the fallback logic complexity in finding the engine. Design-wise, the one-way import dependency from automation/ to mlc/ is emphasized to avoid circular imports. |
| → find_target_folder("script") # walks registered repos | ||
| → find_target_folder("script") # bundled automation/script/ first, | ||
| # falls back to scanning registered repos | ||
| → dynamic_import_module(module.py) # loads ScriptAutomation |
There was a problem hiding this comment.
Good clarification on the fallback order for find_target_folder; consider explicitly documenting the fallback behavior in the function's docstring for developer clarity.
| too — it was **not** removed there, kept for backward compatibility — but | ||
| `find_target_folder()` prefers the bundled copy in this repo, so that copy is | ||
| no longer what runs. Script *content* (the 377+ `script/<alias>/` directories | ||
| with `meta.yaml`, `customize.py`, `run.sh`) still lives in `mlperf-automations` |
There was a problem hiding this comment.
The version drift risk is well explained; consider adding a lint or CI check to warn if mlperf-automations/automation/ is edited to avoid contributor confusion.
| `mlperf-automations` repo and is pulled/cloned on demand — only the engine | ||
| that runs that content moved here. This file covers everything an AI agent | ||
| needs to contribute correctly to this repo. | ||
|
|
There was a problem hiding this comment.
The updated description of repo roles is clearer and more accurate; this will help new contributors understand the split between engine and content.
|
|
||
| mlcflow does **not** contain or execute benchmark logic. It finds the engine, | ||
| loads it, and hands off `run_args`. | ||
| - `mlcflow` (this repo) — the CLI driver *and* the script execution engine: |
There was a problem hiding this comment.
The note about bundled_automation_path() and fallback scanning is important; consider adding a diagram or flowchart to visually represent this resolution order.
| @@ -47,7 +69,8 @@ loads it, and hands off `run_args`. | |||
| mlcflow/ | |||
| mlc/ | |||
There was a problem hiding this comment.
The explanation of dynamic_import_module's sys.path manipulation is helpful; ensure this approach is robust across different Python environments and document any limitations.
| automation/ # ScriptAutomation ENGINE (migrated from mlperf-automations) | ||
| utils.py | ||
| script/ | ||
| module.py # ScriptAutomation — full script execution logic |
There was a problem hiding this comment.
The guidance on avoiding circular imports is critical; consider adding automated checks or guidelines in the developer docs to enforce this pattern.
| @@ -0,0 +1,926 @@ | |||
| # MLC Script Automation — How It Works | |||
There was a problem hiding this comment.
The addition of a comprehensive README for the automation script engine is excellent for onboarding and maintenance.
Summary
automation/engine (ScriptAutomation:module.py,cache_utils.py,docker.py,apptainer.py, etc. — ~12,000 lines) frommlcommons@mlperf-automationsinto this repo atautomation/, shipped as part of themlcflowpackage.mlc/action.pygainsbundled_automation_path();find_target_folder()now prefers the bundled engine and only falls back to scanning registered repos if it's absent (preserves the existing dev-override path).pyproject.tomlshipsautomation/automation.scriptas packages, including non-.pydata files (meta.json,README.md).AGENTS.md/.claude/skill.mdupdated to describe the engine as bundled rather than externally loaded.tests/test_automation_bundled.pycovering bundled-path resolution, dynamic import ofScriptAutomation, and the registered-repo fallback path.mlcommons@mlperf-automationskeeps its ownautomation/folder for now — it has not been removed there. mlcflow'sfind_target_folder()prefers the bundled copy in this repo, so that copy inmlperf-automationsis simply no longer what actually runs; whether to remove it is a separate, later decision.Test plan
python -m pytest tests/— 11/11 passing (7 pre-existing + 4 new)python -m build) and installed into a clean venv — confirmedautomation/(incl.meta.json/README.md) is packaged and importablemlcr detect,os --quietend-to-end against a clean install, confirming the bundled engine works correctly on its own~/MLC/repossetup via an editable dev install🤖 Generated with Claude Code