Skip to content

fix(spawn): emit a logdir-complete context on both spawn channels, and make context_with_logdir public - #79

Open
rhoadesScholar wants to merge 1 commit into
v2.0from
jeffr/spawn-context-carries-logdir
Open

fix(spawn): emit a logdir-complete context on both spawn channels, and make context_with_logdir public#79
rhoadesScholar wants to merge 1 commit into
v2.0from
jeffr/spawn-context-carries-logdir

Conversation

@rhoadesScholar

Copy link
Copy Markdown
Contributor

What breaks today

daisy 1.x guaranteed that every worker-visible context carried logdir. v2 injects it only inside the private subprocess wrapper (_worker_processes.context_with_logdir, added in 505e7be), so the spawn boundary's two consumer-visible channels still emit five-key, logdir-less contexts:

  1. the process-global DAISY_CONTEXT env var written at spawn (daisy-py/src/py_callbacks.rs) — whose own comment promises that "the canonical cluster-worker pattern" keeps working;
  2. the keyword-only context argument handed to spawn functions.

And absence is not an error: Client silently adopts the worker's own default when the key is missing (daisy-py/python/daisy/_task.py:336-341), so a consumer reading either channel gets per-worker ./daisy_logs scatter with no failure anywhere. Downstream this is load-bearing beyond log placement — volara keys its blocks_done done-marker store under the basedir, so a worker on the wrong basedir orphans every block while the driver looks healthy (context: the review discussion on e11bio/volara#62).

The fix

  • py_callbacks.rs: complete the context once, in PySpawnWorker::spawn, at the last point both channels share — the Rust core cannot do it (the log directory is a Python-side daisy.logging global), and any later point misses the env-var channel. Both channels now emit the same, full context, restoring the 1.x contract.
  • daisy.logging.context_with_logdir is now public (also re-exported at top level, like get_worker_log_basename): anything relaying a context onward — a spawn function wrapping srun/sbatch/docker run, or a downstream test pinning this contract — can produce exactly the context a spawned worker receives without importing daisy._worker_processes. The private name stays as an alias so existing imports keep resolving.

Semantics preserved: an empty logdir still encodes "file logging off" (set_log_basedir(None)), and Client still maps it back. One deliberate refinement: the public helper respects an existing logdir value instead of overwriting it — an explicit upstream choice beats the relaying process's global. At daisy's own call sites the two behaviours produce the identical value (the same global, read moments apart).

Reproduction

Two new tests in tests/test_worker_execution.py, run both ways:

On v2.0 @ b323d78 (stock):

$ pytest tests/test_worker_execution.py -k "env_var_channel or is_public" -q
E   KeyError: 'logdir'                                                  # the env-var channel after a run
E   AttributeError: module 'daisy' has no attribute 'context_with_logdir'
2 failed

On this branch:

$ uv run pytest tests/test_worker_execution.py -k "env_var_channel or is_public" -q
2 passed

Full suite on the branch: 2 failed, 241 passed, 2 skipped, 1 xfailed — the 2 are test_block_function_gets_real_parallelism and test_inline_zero_arg_worker_gets_real_parallelism, and they fail identically on stock b323d78 on the same host (a 2-vCPU cloud node; they are hardware-parallelism assertions). Not introduced here. Environment: uv sync --locked --extra worker-processes + dev group, Python 3.13, rustc stable; funlib.geometry added for test_block_compat.py collection (it hard-imports funlib but is absent from the dev group — pre-existing, worth a separate look).

Lint: the repo's lint workflow is red on v2.0 itself (28 pre-existing I001s, failing runs since 2026-08-04). The four Python files touched here are ruff format-clean and this change adds zero new ruff check violations (the one hit in tests/test_worker_execution.py is the file's pre-existing top-of-file I001, untouched).

Scope notes

  • A pure-Rust runner (no embedded Python) cannot know the Python-side logging global; that path is out of scope here and unchanged.
  • The existing spawn-path tests (test_zero_arg_worker_sees_the_parent_log_basedir, test_re_execed_worker_still_gets_the_log_dir, the :-separator and logging-off tests) are untouched and still pass — the wrapper's injection is now redundant on the server path rather than load-bearing, and stays as belt-and-braces.

Opened as a draft per our review flow; prompted by @pattonw's review on e11bio/volara#62.

🤖 Generated with Claude Code

https://claude.ai/code/session_017QGnabZYYcDs5NLMtnMq8t

…ke context_with_logdir public

daisy 1.x put logdir on every worker-visible context. v2 injected it only
inside the private subprocess wrapper, so the spawn boundary's two
consumer-visible channels -- the process-global DAISY_CONTEXT env var
(whose own comment promises 1.x cluster-worker compat) and the
keyword-only context argument -- both emitted five-key, logdir-less
contexts, and Client silently adopts the worker's own default when the
key is absent. Complete the context once in PySpawnWorker::spawn, the
last point both channels share; promote context_with_logdir to
daisy.logging (top-level re-export, private alias kept).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017QGnabZYYcDs5NLMtnMq8t
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant