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
Open
Conversation
…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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:DAISY_CONTEXTenv var written at spawn (daisy-py/src/py_callbacks.rs) — whose own comment promises that "the canonical cluster-worker pattern" keeps working;contextargument handed to spawn functions.And absence is not an error:
Clientsilently 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_logsscatter with no failure anywhere. Downstream this is load-bearing beyond log placement — volara keys itsblocks_donedone-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, inPySpawnWorker::spawn, at the last point both channels share — the Rust core cannot do it (the log directory is a Python-sidedaisy.loggingglobal), 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_logdiris now public (also re-exported at top level, likeget_worker_log_basename): anything relaying a context onward — a spawn function wrappingsrun/sbatch/docker run, or a downstream test pinning this contract — can produce exactly the context a spawned worker receives without importingdaisy._worker_processes. The private name stays as an alias so existing imports keep resolving.Semantics preserved: an empty
logdirstill encodes "file logging off" (set_log_basedir(None)), andClientstill maps it back. One deliberate refinement: the public helper respects an existinglogdirvalue 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):On this branch:
Full suite on the branch:
2 failed, 241 passed, 2 skipped, 1 xfailed— the 2 aretest_block_function_gets_real_parallelismandtest_inline_zero_arg_worker_gets_real_parallelism, and they fail identically on stockb323d78on 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.geometryadded fortest_block_compat.pycollection (it hard-imports funlib but is absent from thedevgroup — pre-existing, worth a separate look).Lint: the repo's
lintworkflow is red onv2.0itself (28 pre-existingI001s, failing runs since 2026-08-04). The four Python files touched here areruff format-clean and this change adds zero newruff checkviolations (the one hit intests/test_worker_execution.pyis the file's pre-existing top-of-fileI001, untouched).Scope notes
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