Support --mlc_isolate

[amd-arsuresh]

✅ PR Checklist

✅ Testing & CI

• Have tested the changes in my local environment, else have properly conveyed in the PR description
• The change includes a GitHub Action to test the script(if it is possible to be added).
• No existing GitHub Actions are failing because of this change.

📚 Documentation

• README or help docs are updated for new features or changes.
• CLI help messages are meaningful and complete.

📁 File Hygiene & Output Handling

• No unintended files (e.g., logs, cache, temp files, pycache, output folders) are committed.

🛡️ Safety & Security

• No secrets or credentials are committed.
• Paths, shell commands, and environment handling are safe and portable.

🙌 Contribution Hygiene

• PR title and description are concise and clearly state the purpose of the change.
• Related issues (if any) are properly referenced using Fixes # or Closes #.
• All reviewer feedback has been addressed.

[github-actions]

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

[anandhu-eng]

This review was performed by an AI coding agent (Claude), at the request of the repository maintainer, by reading the PR diff and tracing the callers and callees of the changed code in a local checkout (mlc/action.py, mlc/cache_action.py, automation/script/module.py) at PR head commit b77e431e. It is intended as input to human review, not a substitute for it.

Verdict

PR #309 adds a --mlc_isolate flag meant to run a script against a fresh, disposable MLC_REPOS so it can't touch — or be touched by — the real one. It restores the environment variable and working directory afterward, but the isolation itself is incomplete: the script cache, which gets written to on essentially every run, is never repointed at the isolated directory and keeps hitting the real, shared one instead. This should be fixed before merge — as written, the flag doesn't deliver what its own description promises.

🔴 High severity

Cache reads/writes during an "isolated" run still hit the real, shared cache, not the isolated one

_run_isolated (mlc/script_action.py:534-576 at head b77e431e) builds a fresh Action() pointed at the isolated folder and copies its fields onto self via self.__dict__.update(vars(new_parent)) (mlc/script_action.py:565). That call does not set self.parent — Action.__init__ (mlc/action.py:233-276) never assigns self.parent on a bare Action(), so vars(new_parent) has no parent key, and self.parent is left exactly as ScriptAction.__init__ (mlc/script_action.py:43-45) originally set it: the real, non-isolated Action.

That matters because the engine's cache handle is built from that same reference: automation/script/module.py:39 — self.cache_action = CacheAction(self.action_object.parent), where self.action_object is the mutated self, but .parent on it is still the original. CacheAction.search()/.rm() (mlc/cache_action.py:50,106) delegate straight to self.parent.search(...)/self.parent.rm(...). Two call sites confirm cache writes happen mid-run: automation/script/module.py:1173 and :1782, both self.cache_action.access({'action': 'update', 'target': 'cache', ...}).

Net effect: a run made with --mlc_isolate can read pre-existing entries from the real ~/MLC/repos cache and will write new ones back into it — the opposite of "fresh MLC_REPOS" as the flag's own docstring (mlc/script_action.py:521) describes it.

Fix: set self.parent to the isolated Action (and restore the original in finally) alongside the __dict__.update, so everything the engine derives from self.action_object.parent — including CacheAction — resolves against the isolated directory too.

🟠 Medium severity

Isolated state on self is never restored after the run — only the env var and cwd are

Same function, mlc/script_action.py:568-576. finally restores os.chdir(orig_dir) and MLC_REPOS, but never restores self.repos_path / self.repos / self._index / self.local_cache_path, overwritten in place at line 565. A one-shot mlc/mlcr CLI call exits right after, so this doesn't surface there — but mlcflow's own documented Python API (AGENTS.md: a = Action(); a.access(...) called repeatedly, or any orchestration loop reusing one instance) would silently keep using the isolated directory on the next call, which may already be deleted if --mlc_isolate_clean was set.

Fix: snapshot dict(self.__dict__) before the mutation and restore it in finally, the same way the env var and cwd already are.

No test exercises the new flag, and CI would not have caught either issue above

gh pr view 309 --json files shows only mlc/script_action.py changed — no test file. mlc/** is in the path filter for test-unit.yml and test-mlc-script-features.yml, so CI does run on this diff, but grep -rn "mlc_isolate" .github/ tests/ returns nothing — neither workflow exercises the flag, so both findings above pass CI silently.

Fix: add a test that runs a script with --mlc_isolate, writes to cache during it, and asserts the entry lands in the isolated directory, not the real ~/MLC/repos cache.

🟡 Low severity

PR description is the unfilled checklist template

Every checkbox is unchecked and there's no free-text description of what was manually tested. Not a functional issue, but there's no stated evidence the isolated-run path was exercised at all.