Conversation
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
c3cbdb8 to
0eb2299
Compare
…he step Pyxis creates the container *inside* the `srun` step, so Enroot reads `ENROOT_TEMP_PATH` and `ENROOT_CONFIG_PATH` there and not in the service process. Neither was on the step environment allow-list, so both were silently dropped. The consequence is not a failed step, which is why it took so long to see: an operator points `ENROOT_TEMP_PATH` at a large device precisely so that unpacking a ~2.5 GB rootfs with ~16.8k hardlinks does not compete for space with the unpacked rootfs itself, the override never arrives, and the create-time temp lands back on the very device it was meant to spare. On a 20-node run this is how `/raid` reached 4.1 GB free of 527 GB, after which every subsequent container creation failed for want of space -- reported as an ordinary infrastructure failure with no mention of the setting that was discarded. This is the same class as the proxy variables already on the list and for the same structural reason: work that looks like it happens in the service actually happens in the step, and configuration that does not cross that boundary is configuration that does nothing. No other `SLURM_*` variable is added; inheriting `SLURM_JOB_ID` / `SLURM_STEP_ID` is what breaks a nested `srun` and is why the allow-list exists. Kept deliberately separate from the `SLURM_CONF` + proxy commit rather than folded into it. That commit is also PR #452 upstream; if #452 merges on its own and the stack drops its commit, this fix has to survive that, and it only does if it stands alone. Tests: the existing allow-list parametrisation gains both variables (each fails against the previous list), and `test_pyxis_srun_environment_withholds_inherited_step_identity` continues to pin the other half of the contract. The service README documents the allow-list as a table with the reason each load-bearing entry is on it.
…reated `PyxisEnvironment.cleanup()` never reclaimed anything. Pyxis namespaces named containers by the allocation, so `--container-name=X` inside job `N` is the Enroot container `pyxis_N_X`. cleanup() asked for `pyxis_X`, which does not exist. `enroot remove` exited non-zero, and `check=False` with `capture_output=True` discarded both the status and the message, so the failure was invisible. Symptom: nothing is reclaimed for the life of an allocation. Measured on a 20-node run -- 199 trajectory rootfs coexisting on one node and `/raid` down to 4.1 GB free of 527 GB, after which every subsequent container creation failed for want of space. It is also the origin of the "scancel doesn't reap enroot containers" folklore: `scancel` genuinely does not remove Enroot containers, but the containers here were never asked to go away in the first place, so the blame landed on SLURM. Two changes: * `enroot_container_name(job_id, name)` builds the name Pyxis actually created, and cleanup() uses it. * A non-zero `enroot remove` is logged with its stderr instead of being swallowed, so the next time this path breaks it says so. Tests: `test_pyxis_cleanup_removes_the_container_pyxis_actually_created` models an Enroot container set and asserts the created container is the one removed (it fails against the old name, which removes nothing); `test_pyxis_cleanup_reports_a_removal_that_did_not_happen` asserts the warning; `test_enroot_container_name_is_namespaced_by_job` pins the naming rule. The existing container-reuse test asserted the unnamespaced form and is corrected.
…ork queue Adds the two foundations of the distributed SWE-bench harness: - units.py: shards an instance-id list into immutable, content-addressed units. The sha256 digest covers the ordered id list, so a plan cannot be silently reused across a different run, instance list, or ordering. - queue.py: a filesystem work queue whose claim is a bare os.mkdir (never makedirs(exist_ok=True), which hands a unit to every caller). available() is plan - claims - results, so deleting a result alone does NOT requeue a unit; requeue() is the only supported path and removes the result, the claim and the attempt records together. Env faults are ledgered separately from counted attempts, and abandoning a unit publishes a terminal result AND releases the claim so claims/ and results/ never disagree.
0eb2299 to
33279b8
Compare
arekay-nv
left a comment
There was a problem hiding this comment.
Can you update the documentation (either PR description or the Readme) to indicate the lifecycle of the unit/plans/queue. The queue doesn't seem to be plugged in, so the workflow cannot be understood.
| if os.environ.get("SLURM_JOB_ID", "").strip(): | ||
| job_id = os.environ.get("SLURM_JOB_ID", "").strip() | ||
| if job_id: | ||
| container = enroot_container_name(job_id, self.name) |
There was a problem hiding this comment.
minor: container_name would be more appropriate here.
|
|
||
| import msgspec | ||
|
|
||
| PLAN_FILENAME = "units.json" |
There was a problem hiding this comment.
Should move this as a class variable under UnitPlan - makes it more robust to updates.
| if not run_id or "/" in run_id or run_id in {".", ".."}: | ||
| raise PlanError(f"invalid run_id: {run_id!r}") |
There was a problem hiding this comment.
Please add docs explaining why these checks are necessary - if run_id is expected to be a folder name adding that constraint here would be useful for users.
| if not ordered: | ||
| raise PlanError("cannot plan a run with no instance ids") |
There was a problem hiding this comment.
Suggest having
if not instance_ids:
raise PlanError("cannot plan a run with no instance ids")
at the start. Unless the ordering can result in getting an empty list from a non-empty instance_ids.
| units: list[Unit] = [] | ||
| for shard, start in enumerate(range(0, len(ordered), shard_size)): | ||
| chunk = tuple(ordered[start : start + shard_size]) | ||
| units.append( | ||
| Unit( | ||
| unit_id=f"{run_id}.s{shard:02d}", | ||
| run_id=run_id, | ||
| shard=shard, | ||
| instance_ids=chunk, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Simpler to use itertools.batched:
from itertools import batched
units = [
Unit(
unit_id=f"{run_id}.s{shard:02d}",
run_id=run_id,
shard=shard,
instance_ids=chunk,
)
for shard, chunk in enumerate(batched(ordered, shard_size))
]
| ) | ||
|
|
||
|
|
||
| def read_plan(path: Path) -> UnitPlan: |
There was a problem hiding this comment.
This should be a class method.
| EVIDENCE_FILES = ( | ||
| "status.json", | ||
| "swe_bench_results.json", | ||
| "preds.json", | ||
| "swe_bench_service_status.json", | ||
| ) |
There was a problem hiding this comment.
Please add short comments to each file name.
| def to_dict(self) -> dict[str, Any]: | ||
| return { | ||
| "unit_id": self.unit_id, | ||
| "run_id": self.run_id, | ||
| "plan_digest": self.plan_digest, | ||
| "outcome": self.outcome.value, | ||
| "accounted_instance_ids": list(self.accounted_instance_ids), | ||
| "resolved_instance_ids": list(self.resolved_instance_ids), | ||
| "infra_error_count": self.infra_error_count, | ||
| "genuine_error_count": self.genuine_error_count, | ||
| "error_kinds": dict(self.error_kinds), | ||
| "service_url": self.service_url, | ||
| "endpoint_fingerprint": self.endpoint_fingerprint, | ||
| "service_run_id": self.service_run_id, | ||
| "attempt": self.attempt, | ||
| "abandoned": self.abandoned, | ||
| "duration_s": self.duration_s, | ||
| "detail": self.detail, | ||
| "finished_at": self.finished_at, | ||
| } | ||
|
|
||
| @classmethod | ||
| def from_dict(cls, raw: dict[str, Any]) -> UnitResult: | ||
| return cls( | ||
| unit_id=str(raw["unit_id"]), | ||
| run_id=str(raw["run_id"]), | ||
| plan_digest=str(raw["plan_digest"]), | ||
| outcome=UnitOutcome(str(raw["outcome"])), | ||
| accounted_instance_ids=tuple( | ||
| str(x) for x in raw.get("accounted_instance_ids") or () | ||
| ), | ||
| resolved_instance_ids=tuple( | ||
| str(x) for x in raw.get("resolved_instance_ids") or () | ||
| ), | ||
| infra_error_count=int(raw.get("infra_error_count") or 0), | ||
| genuine_error_count=int(raw.get("genuine_error_count") or 0), | ||
| error_kinds=dict(raw.get("error_kinds") or {}), | ||
| service_url=raw.get("service_url"), | ||
| endpoint_fingerprint=raw.get("endpoint_fingerprint"), | ||
| service_run_id=raw.get("service_run_id"), | ||
| attempt=int(raw.get("attempt") or 0), | ||
| abandoned=bool(raw.get("abandoned")), | ||
| duration_s=float(raw.get("duration_s") or 0.0), | ||
| detail=raw.get("detail"), | ||
| finished_at=float(raw.get("finished_at") or 0.0), | ||
| ) |
There was a problem hiding this comment.
Suggestion to use msgspec if not too much work. Can also add as followup.
| return Path("/proc/sys/kernel/random/boot_id").read_text().strip() | ||
| except OSError: | ||
| try: | ||
| return str(int(time.time() - time.monotonic())) |
There was a problem hiding this comment.
This seems like an arbitrary fallback. Can you add some comments explaining this.
| self.env_failed_dir, | ||
| self.artifacts_dir, | ||
| ): | ||
| directory.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
For user specified report_dir this can potentially overwrite the root directory.
Adds content-addressed work units and a mkdir-atomic durable queue so distributed SWE-bench runs can resume safely after client failures.
Also makes Pyxis launch configuration, environment forwarding, and container cleanup deterministic, with coverage for unit planning and queue lifecycle.
Dependency base: swe-dist-dependencies contains the canonical patches from open PRs #453, #454, and #456. This base will collapse back to main after those PRs merge; merged PR #452 is already inherited from main.