Streamline local compute launch with readiness waiting and policy controls - #232
Conversation
✅ Eval
lc statusConfusion & pain points (Claude analysis)Confusion & pain points
Full trace: |
EiffL
left a comment
EiffL
left a comment
There was a problem hiding this comment.
Automated review of 85b8d8d + 016e522 against main (15 findings; tests/test_compute.py passes). Four were reproduced by running the code, as marked; the rest come from reading it.
Most findings come from two changes:
1. The built-in local offer is now merged into configured catalogs. A Slurm-only catalog can silently fall back to a whole-host local cluster, a local discovery error can block Slurm launches, and a remote offer named local makes the catalog invalid. This also contradicts the layer-7 invariant in CLAUDE.md, the lc compute --help text and the getting-started guide. Deciding whether configured catalogs should get the local offer at all settles most of this group.
2. The per-user lock at /tmp/lightcone-local-<uid>. An error raised during launch is reported as a lock failure. An interrupted launch can leave an undiscoverable owner holding the lock. The refusal doesn't name the owning cluster. Another user can pre-create the directory, and /tmp cleaners can unlink a held lock. Tests can't redirect it, and a stdio-closed launcher can lose the lock to DEVNULL. Moving the lock under the connection root and writing the owner's cluster ID into it would address several of these at once.
Smaller items: leftover code for pre-lock allocations, the local-disabled check duplicated in four places, and a cleanup assertion dropped from the failed-spawn test.
🤖 Generated with Claude Code
| resources = self.local.resources or Resources.from_bytes( | ||
| cpus=CPU_COUNT, memory_bytes=MEMORY_LIMIT, | ||
| ) | ||
| offers.append(Offer( |
There was a problem hiding this comment.
Remote-only catalogs silently gain a whole-host local offer (reproduced)
A request that no remote offer satisfies now falls through to a local allocation instead of failing.
Scenario: an existing Slurm catalog with no local: block, on a login node. lc compute launch --cpus 2+ --memory 1+ --startup fast skips the Slurm offer (startup class unknown) and plans offer=local on connection=local with every detected CPU and all RAM (confirmed with --dry-run). The same happens when a Slurm offer raises UnavailableOfferError. The researcher gets a LocalCluster on the login node sized to the whole machine rather than a "no configured offer matches" error. Before this change, configured catalogs replaced the built-in offer completely.
| "lock_fd": lock_fd, | ||
| }, | ||
| ) | ||
| return identity |
There was a problem hiding this comment.
An interrupted launch orphans an owner that holds the lock
The except Exception just below does not catch KeyboardInterrupt, and nothing catches SIGTERM/SIGKILL. If the launcher is interrupted after Popen but before identity.json/launch.json are published (for example, Ctrl-C or an agent timeout during the fsync'd writes), there is no killpg. The owner's loop (local_runtime.py:52) polls for an identity until its itimer fires, which is 30 min to 2 h, or longer for explicit offers.
Discovery skips the directory because it has no identity.json, so lc compute down has nothing to address, yet every lc compute launch fails with "already running or starting… stop it with lc compute down". Before the lock existed this orphan did no harm.
| try: | ||
| fcntl.flock(descriptor, fcntl.LOCK_EX | fcntl.LOCK_NB) | ||
| except BlockingIOError as exc: | ||
| raise ComputeError( |
There was a problem hiding this comment.
The refusal names no cluster, so the suggested remedy often can't be followed
The lock covers every catalog and connection root, but discovery only sees the current catalog.
Scenario: a cluster is launched with LC_COMPUTE_CONFIG pointing at an explicit local connection (custom connection_root or namespace), or with the built-in namespace before the user switches to their own local namespace. In a shell using the other catalog, lc compute launch says "a local cluster is already running… stop it with lc compute down", but lc compute status prints "No allocations found." and down cannot resolve it ("namespace is absent from the compute catalog"). The user is blocked until walltime. evals/prompt.md tells agents to "inspect lc compute status and reuse it", which cannot work here.
Writing the owner's cluster ID into the lock file would let the refusal name it.
| "stop it with lc compute down before launching another" | ||
| ) from exc | ||
| yield descriptor | ||
| except OSError as exc: |
There was a problem hiding this comment.
except OSError wraps the yield, so launch-body errors are reported as lock failures (reproduced)
When writing launch.json fails with EDQUOT (home quotas are common on HPC), the error reads cannot acquire the local allocation lock: [Errno 122] Disk quota exceeded, and the token directory under the connection root is left behind. A PermissionError from discover()'s iterdir is mislabeled the same way.
Only the os.open/fstat/flock calls should be inside the OSError handler.
| "connection name 'local' is reserved for the built-in local backend" | ||
| ) | ||
| # Retain this authority even when disabled so existing allocations can be stopped. | ||
| connections["local"] = Connection(namespace=_LOCAL_NAMESPACE, provider="local") |
There was a problem hiding this comment.
The implicit local connection is added even with local.enabled: false, so a local discovery error now blocks Slurm
Scenario: a Slurm-only catalog on a machine where ~/.lightcone/compute/22c84e48-… has a permission problem (for example, restored from backup, so private_directory refuses), or where a local process identity cannot be verified. Compute.discover() records an error under local. Compute.launch then refuses every Slurm launch ("cannot check cluster names while discovery is incomplete: local: …"), and lc materialize my-slurm-cluster cannot resolve the name. Remote-only catalogs could not hit this before.
| patch.setattr("lightcone.engine.compute.local.subprocess.Popen", fail) | ||
| with pytest.raises(ComputeError, match="cannot execute"): | ||
| _launch(provider) | ||
| # A failed spawn must release the singleton lock as well as its private files. |
There was a problem hiding this comment.
The failed-spawn test no longer asserts that the launcher's private files are removed
The previous assert set(provider.root.iterdir()) == before was dropped when the test was reordered, although the comment still claims the behavior. A regression that leaves the token directory, launch.json or scratch behind on a failed spawn (the case the except OSError comment above already hits) now passes. Only the lock release is checked, indirectly, by the next successful launch.
| if plan.name is not None: | ||
| validate_name(plan.name) | ||
| with _allocation_lock() as lock_fd: | ||
| # Also recognize allocations launched before the lifetime lock existed. |
There was a problem hiding this comment.
CLAUDE.md: backward-compatibility and dead code for pre-lock allocations
CLAUDE.md: "No backward-compatibility code. Nothing exists to honor the behavior of an older CLI…" and "No dead code."
This comment and the if self.discover(): below run an extra discovery on every launch whose only purpose is allocations launched before the lock existed. Likewise local_runtime.py:28 if "lock_fd" in launch: is always true, since both launch.json writes now include lock_fd.
| "local.resources cannot be combined with explicit local connections; " | ||
| "set their offer resources instead" | ||
| ) | ||
| if not explicit: |
There was a problem hiding this comment.
CLAUDE.md: a recorded layer-7 invariant is reversed without being updated
CLAUDE.md still says the built-in catalog is "one CPU, 1 GiB" and "Configured catalogs replace it completely", and the repository map says catalog.py # compute.yaml, or the built-in local offer. _with_local now sizes the offer to the whole host and merges it into configured catalogs. CLAUDE.md says reversals "land as Recorded decisions here", so as it stands the next contributor reads the opposite of the current behavior.
| Launch the built-in local offer; no compute configuration is needed. It provides | ||
| one CPU and 1 GiB for 30 minutes. Keep the returned ID in `CLUSTER` for this | ||
| all usable CPUs and RAM for 30 minutes. Keep the returned ID in `CLUSTER` for this | ||
| walkthrough. If you already have a compute catalog, its offers replace that default; |
There was a problem hiding this comment.
This line and the lc compute --help text still say a configured catalog replaces the local offer
This sentence, and the compute group docstring in src/lightcone/cli/compute.py:62 ("The catalog is LC_COMPUTE_CONFIG, else ~/.lightcone/compute.yaml, else a built-in local offer"), are now false: the local offer is appended to configured catalogs unless explicit local connections exist or local.enabled: false. The docs rule is to document only what exists.
| ) -> LaunchPlan | None: | ||
| """Match one shape and validate its provider without allocating anything.""" | ||
| connection = self.catalog.connections[offer.connection] | ||
| if connection.provider == "local" and not self.catalog.local.enabled: |
There was a problem hiding this comment.
The local-disabled policy is special-cased in four places, one of them dead
provider == "local" and not catalog.local.enabled appears in _plan_offer, plan_local, launch and connect, with the message string copied three times. The _plan_offer branch can never fire because Catalog._with_local already drops disabled local offers. A new execution entry point that skips compute.connect (for example, Compute.status, which calls provider.connect directly) silently escapes the policy. Enforcing it once, in Compute.provider or as a connection-level flag set by _with_local, would remove the copies.
Make detached local startup transactional with a parent-child pipe, clean unpublished failures, and preserve published identities. Protect the per-user lock, retain owner diagnostics, and handle closed stdio and the brief kernel teardown window after expiry. Disable local launch and execution on recognized NERSC login nodes while permitting interactive compute nodes and retaining Slurm access, inspection, and termination. Clarify catalog errors, isolate lifecycle tests, and update the documentation. Validation: 249 compute, local lifecycle, and output tests passed; Ruff, mypy, and git diff --check passed. Earlier Slurm and CLI validation passed with one bootstrap test passing on retry. No live NERSC allocation was tested; the documented home-filesystem flock limitation remains.
The one-local-cluster-per-user rule was an flock held in the account home
for the owner's lifetime, and NERSC home filesystems do not support flock.
A launch now scans the process table for a session leader of this user
running the owner command, which spans every catalog and connection root
with no file lock. The owner command is one shared constant for launch,
the scan and the identity check.
A refusal still names the running cluster and the catalog to stop it
with, now recorded in its identity file. A record not yet written means
the owner is starting ("retry shortly"); a missing or unreadable one
never recovers, so that refusal names the owner's PID. An unreadable
process table is a clean ComputeError.
Accepted: overlapping launches can both start, and the scan covers one
PID namespace. The suite scopes the scan to its own temporary tree, so a
developer's running cluster does not refuse test launches.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 participant
Starting local compute currently requires CPU and memory flags, a separate readiness command, and a custom catalog for anything larger than 1 CPU / 1 GiB.
lc compute launch --waitnow creates a cluster namedlocalusing the machine's detected usable CPUs and RAM and returns when it is ready for execution.launch --waitand--timeout, retaining the accepted cluster ID on timeout or startup failure without resubmitting or terminating it.local.resourcesto override the default CPU/RAM budget andlocal.enabled: falseto block local launches and execution. Inspection and termination remain available.lc compute launch --waitand explains cluster reuse, timeout handling, resource overrides, and disabled-local policy.The default local lifetime remains 30 minutes (two-hour maximum), and GPU offers remain explicit. CPU/RAM budgets remain cooperative scheduling limits.
Validation:
launch --waitreopened from another process.ruff check src/ tests/andmypy src/passed.git diff --checkpassed. Slurm validation uses simulated native commands; no live Slurm allocation was submitted.