Deterministic run layout and {run_dir} placeholder by atnair-amd · Pull Request #355 · ROCm/cvs

atnair-amd · 2026-08-20T03:08:48Z

Summary

Every rank of a Slurm/SPUR job step has to derive the same run directory without coordinating, because those paths are the rendezvous for the agent control plane: worker ranks never enter pytest and read rank0's port out of agent_dir. RunLayout resolves workspace / run_dir / agent_dir once in cvs run, before pytest or agent bootstrap, and is the single source of truth for those paths thereafter.

Ticket: Fixes AIMVT-300 (story AIMVT-297)

Change

--workspace, falling back to $CVS_WORKSPACE then to the venv's parent directory. Run directory is <workspace>/cvs_runs/<run_id>.
run_id is the scheduler's job id, else local-<timestamp>. Taken as-is: it is the one name every rank already agrees on, and how finely a job subdivides into runs is a question for whatever needs that granularity, not for the module that holds the paths.
Managed-or-not comes from is_managed_compute() (AIMVT-298), so the layout cannot disagree with the rest of CVS about what kind of run this is. Where the scontrol/spur probe cannot see the scheduler — the CVS container image ships neither — CVS_SCHEDULER is the existing override; without it the run is unmanaged and each rank would take its own timestamp.
{run_dir} placeholder for test configs, read off the layout. cvs/lib/utils_lib.py imports RunLayout inside the function rather than at module level: importing cvs.core.run_layout pulls in cvs/core/__init__.py, whose orchestrator factory reaches cvs/core/orchestrators/baremetal.py, which imports utils_lib back. Deferring to call time is enough — pytest.main() runs in-process, so the resolver sees the same layout object cvs run built. The resolver only asks for a layout when the token is actually present: RunLayout.get() creates directories, and roughly half the test modules call it with configs that never mention {run_dir}.
Out of scope: passing the run directory through to the container orchestrator, which stacks on this branch as Mount the run directory into orchestrated containers #356; and the agent bootstrap that consumes agent_dir (AIMVT-297).

Tests

31 unit tests: cvs/core/unittests/test_run_layout.py (18), cvs/cli_plugins/unittests/test_run_plugin.py (9), cvs/lib/unittests/test_utils_lib.py (4). They cover workspace precedence, rank agreement, unwritable workspaces, the no-venv case, that the derived default composes the same path shape as an explicit workspace, that an environment disagreeing with the layout cannot redirect where artifacts land, and that a mistyped suite name exits without creating a run directory. Run-id tests pin CVS_SCHEDULER so they do not depend on whether the host has scheduler binaries installed. What makes a run "managed" is cvs.core.scheduler's contract and is tested there, not re-tested through this module.

Gate: make fmt-check, make lint (pylint 10.00/10), make ut — 1226 tests, all passing.

Every rank of a Slurm/Spur job step must derive the same shared-filesystem paths: worker ranks never enter pytest, they read rank0's port out of agent_dir. RunLayout resolves workspace/run_dir/agent_dir once, before pytest or agent bootstrap, from inputs every rank shares. - cvs/core/run_layout.py: the layout, cached per process so a timestamped run_id cannot drift between callers. - cvs run --workspace, resolved after the test name validates so a mistyped suite does not litter shared storage. - {run_dir} in test configs, published as CVS_RUN_DIR. utils_lib reads the environment rather than importing run_layout, which would be circular via cvs/core/__init__.py -> orchestrator factory -> utils_lib. run_id keys off the job-step environment rather than is_managed_compute(): that predicate also probes for the scontrol/spur binaries, which are absent from the container image even when srun exported the SLURM_* variables, so ranks would each fall back to their own wall clock and diverge. It includes SLURM_STEP_ID and the restart count because the job id alone is not unique per run - concurrent steps in one allocation share it, and a requeue repeats it.

The SPUR/SLURM twin note claimed SPUR exports the SLURM_* variables verbatim. SPUR mirrors each SPUR_* variable it sets, which covers job id, step id and procid but not SLURM_RESTART_COUNT -- SPUR requeues without exporting an attempt counter, so a requeued SPUR job reuses its run directory. The placeholder resolver's note said every suite calls it; 38 of 71 test modules do.

The run id was <job>.<step>[.r<restart>], with CVS deriving a finer run identity than the scheduler's own. Subdividing a job into separate run directories is a question for whatever needs that granularity, not for a module whose job is to hold the paths; the restart component also had no SPUR equivalent, so it only ever worked on one of the two schedulers. Take the job id as-is. Ranks still agree, which is the property the layout exists to provide, and the resolver drops to two branches.

The layout was calling scheduler._running_in_job_step() directly to avoid the binary probe in is_managed_compute(), which an image without scontrol/spur fails. That reached around the module's public API to reimplement half of it, and it broke the CVS_SCHEDULER override in the other direction: forcing bare_metal left the layout still believing it was managed while the rest of CVS did not. CVS_SCHEDULER already covers the undetectable-scheduler case it was working around, so use the public predicate and set that variable where the probe cannot see the scheduler. The tests now pin CVS_SCHEDULER rather than depending on whether the host happens to have the binaries.

Inline the env var names at their use sites. They were module constants only so tests could import them, and a test then asserted each equalled its own literal, which cannot fail. Drop the re-initialize conflict check: initialize() has one call site, so a second call with a different workspace is unreachable. Drop the _new_run_timestamp seam and assert the run id shape instead, so the unmanaged case tests the real value rather than a mock. Drop run-id tests that pin cvs.core.scheduler's contract (CVS_SCHEDULER override, missing binary, salloc without a step); test_scheduler.py owns those.

utils_lib read CVS_RUN_DIR because a module-level import of cvs.core.run_layout is circular. The cycle is real but the import only has to be deferred into the function: by call time cvs.core is imported and nothing is partial. Verified by importing cvs.lib.utils_lib with the module-level import in place, which fails at cvs/core/orchestrators/baremetal.py -- not at the factory, as the old comment claimed. pytest runs in-process via pytest.main(), so the layout resolved in run_plugin is the same object the resolver sees. The env var was a private channel with exactly one reader and could disagree with the layout it was copied from. Drop the CVS_RUN_DIR export with its last reader gone.

cijohnson · 2026-08-21T01:22:35Z

            args.log_level,
            args.capture,
            getattr(args, "extra_pytest_args", []),
+            workspace=args.workspace,


Let us move this after line 74(args.config_file), we no need to maintain default value in run_test for workspace argument because arg parser at line 23 already set the default value to None

when comment [3] is taken care this comment will become invalid

cijohnson · 2026-08-21T01:22:57Z

        log_level,
        capture,
        extra_pytest_args,
+        workspace=None,


we can move it after config_file arg

when comment [3] is taken care this comment will become invalid

cijohnson · 2026-08-21T01:28:08Z

+        # wait for a fixture -- while creating directories for a mistyped suite
+        # name would litter shared storage.
+        try:
+            RunLayout.initialize(workspace)


[3] we should do RunLayout initialization in all the ranks, so we should move to def run method

`
def run(self, args):
try:
RunLayout.initialize(args.workspace) # ALL ranks, before any rank branching
except RuntimeError as e:
print(f"Error: {e}")
sys.exit(1)

self.run_test(...)

`

cijohnson · 2026-08-21T01:29:00Z

        log_level,
        capture,
        extra_pytest_args,
+        workspace=None,


when comment [3] is taken care this comment will become invalid

cijohnson · 2026-08-21T01:29:10Z

            args.log_level,
            args.capture,
            getattr(args, "extra_pytest_args", []),
+            workspace=args.workspace,


when comment [3] is taken care this comment will become invalid

cijohnson · 2026-08-21T01:35:26Z

+    def __init__(self, workspace, run_id):
+        self.workspace = workspace
+        self.run_id = run_id
+        self.run_dir = workspace / "cvs" / "runs" / run_id


lets keep it self.run_dir = workspace / "cvs_runs" / run_id

jira was incorrectly using cvs/runs

cijohnson · 2026-08-21T02:11:32Z

+        self.agent_dir = self.run_dir / "agent"
+
+    @classmethod
+    def initialize(cls, workspace=None):


Consider collapsing initialize() / instance() / instance_or_none() into a single RunLayout.get(workspace=...) — workspace passed → resolve and init idempotently; no args → return existing or None. Same behavior, less API surface.

you can have _initialize but call it from get method.
users of the RunLayout now only need to do one these

RunLayout.get(workspace) # init or return existing (workspace=None → env/default)
RunLayout.get() # existing or None

Review feedback on #355. initialize() / instance() / instance_or_none() were three names for one question. A single idempotent get(workspace=None) answers it: the first caller resolves and creates, later callers get the same layout and their argument is ignored, so no consumer can split a run across two directories. utils_lib now checks for the {run_dir} token before reaching for the layout rather than after. get() creates directories, and roughly half the test modules call this resolver with configs that never mention the token -- resolving a layout for all of them would create a run directory as a side effect of parsing an unrelated config, including inside other modules' unit tests. The cost is that using {run_dir} outside 'cvs run' now resolves a local layout instead of exiting with a diagnostic; 'cvs run' resolves the layout before pytest, so the diagnostic only ever described entry points that do not exist. Run directory is <workspace>/cvs_runs/<run_id>, not <workspace>/cvs/runs/<run_id>.

Worker ranks in a Slurm/Spur job step never enter pytest, so the run layout -- the rendezvous those ranks read -- cannot be resolved inside the handoff that launches it. Move resolution, and the input validation it depends on, up into run(); run_test() now only builds pytest arguments and takes the resolved test file. Input validation and the suite-name lookup stay ahead of the layout so a mistyped suite name still exits without creating a directory on shared storage.

_default_workspace() returned <venv parent>/cvs_runs and run_dir appended cvs_runs again, so a run with no --workspace landed in <venv parent>/cvs_runs/cvs_runs/<run_id> while an explicit workspace got the documented one-level path. The default is now the venv parent itself, which is what the docstring already claimed and what README and the how-to both document. The existing tests pinned the default workspace and pinned run_dir only for an explicit workspace, so nothing asserted the composition of the two. Add that case.

This was referenced Aug 20, 2026

Mount the run directory into orchestrated containers #356

Open

Deterministic run layout and {run_dir} placeholder #354

Closed

atnair-amd self-assigned this Aug 20, 2026

atnair-amd marked this pull request as ready for review August 20, 2026 06:02

atnair-amd requested review from cijohnson and speriaswamy-amd August 20, 2026 06:02

atnair-amd added 6 commits August 19, 2026 23:04

atnair-amd force-pushed the worktree-aimvt-300-core branch from 824b932 to 9701e2e Compare August 20, 2026 06:05

cijohnson reviewed Aug 21, 2026

View reviewed changes

cijohnson requested changes Aug 21, 2026

View reviewed changes

atnair-amd added 3 commits August 20, 2026 19:31

cijohnson approved these changes Aug 21, 2026

View reviewed changes

atnair-amd merged commit f05aa09 into main Aug 21, 2026
2 checks passed

Navigation Menu

Uh oh!

Deterministic run layout and {run_dir} placeholder - #355

Deterministic run layout and {run_dir} placeholder#355
atnair-amd merged 9 commits into
mainfrom
worktree-aimvt-300-core

atnair-amd commented Aug 20, 2026 •

edited

Loading

Uh oh!

cijohnson Aug 21, 2026

Uh oh!

cijohnson Aug 21, 2026

Uh oh!

cijohnson Aug 21, 2026

Uh oh!

cijohnson Aug 21, 2026

Uh oh!

cijohnson Aug 21, 2026 •

edited

Loading

Uh oh!

cijohnson Aug 21, 2026

Uh oh!

cijohnson Aug 21, 2026

Uh oh!

cijohnson Aug 21, 2026

Uh oh!

cijohnson Aug 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

atnair-amd commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Change

Tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cijohnson Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

atnair-amd commented Aug 20, 2026 •

edited

Loading

cijohnson Aug 21, 2026 •

edited

Loading