Skip to content

Loud fail for missing build tools; fix worker payload serialization on editable installs; mode-independent run-stats block counts - #70

Closed
rhoadesScholar wants to merge 3 commits into
v2.0from
v2.0_patch
Closed

Loud fail for missing build tools; fix worker payload serialization on editable installs; mode-independent run-stats block counts#70
rhoadesScholar wants to merge 3 commits into
v2.0from
v2.0_patch

Conversation

@rhoadesScholar

@rhoadesScholar rhoadesScholar commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Three things: a loud, actionable failure when build tooling is missing; a fix for subprocess-worker payload serialization breaking on editable/source-tree installs; and correct blocks_processed run stats across all worker execution modes.

1. Loud fail for missing install tools (357d040)

Building daisy from source requires a C linker and the Rust toolchain, and the stock maturin error when they're missing is cryptic. build_wrapper.py now wraps maturin's PEP 517 hooks (build_wheel / build_editable) and checks for cc/gcc/clang and cargo up front, failing with per-platform install instructions instead. pyproject.toml also gains dev, examples, and docs extras so pip users get the same dependency groups uv users had.

2. Fix: subprocess-worker payloads no longer break on editable installs (ce3f488)

Symptom: with daisy installed editable (or from a source tree), run_blockwise failed at startup with TypeError: cannot pickle '_thread._local' object for any 1-arg block function that references the daisy module as a global (e.g. block.status = daisy.BlockStatus.SUCCESS).

Root cause: subprocess workers (the default execution mode) serialize the block function with dill.dumps(..., recurse=True), which ships the globals the function references. dill pickles any module outside sys.prefix/site-packages by value — its entire __dict__ — so one unpicklable module-level object anywhere in the namespace fails the whole payload. daisy itself contains two (daisy.logging._active, a threading.local, and daisy._worker_processes._LEN, a struct.Struct). Wheel installs never trip this, which is why CI stayed green; editable installs always do.

Fix: _serialize now uses a dill.Pickler subclass that pickles every importable module by reference (_make_modules_by_ref_pickler). This is sound because the child replicates the parent's sys.path before deserializing (read_payload), so anything importable in the parent is importable in the child. Only __main__ (and __mp_main__) keep dill's by-value path — the child's __main__ is the worker shim, so script/notebook globals must ride in the payload, and they still do (recurse=True is load-bearing and untouched). This also fixes the same failure for user packages installed editable with unpicklable module-level state.

Also:

  • daisy.logging._active is now a _PicklableLocal (pickles as a fresh, empty local) as defense-in-depth for other transports.
  • A payload that genuinely cannot be serialized now raises eagerly with guidance (restructure imports, or Task(worker_processes=False)) instead of a bare dill traceback.

3. Fix: blocks_processed counted server-side, correct in every mode (ce3f488)

Symptom: the Resource Utilization report showed blocks 0 per task (and empty per-worker counts) for subprocess-mode runs — the default — and tests/test_run_stats.py had to pin tests to thread mode. This was the documented gap in RUN_STATS.md.

Root cause: WorkerStats.blocks_processed was incremented only inside the in-process worker thread's block loop. Subprocess shim workers and external cluster workers process blocks where the stats layer never looked.

Fix: counting moves to the server, where every worker's block returns already arrive over TCP:

  • New protocol message Register { task_id, worker_id }, sent by Client::connect right after the connection opens (appended as the last enum variant, so existing message discriminants are unchanged on the wire).
  • The bookkeeper records each connection's registered identity; every valid block return (ReleaseBlock or BlockFailed) increments a RunTally counter per task and per worker.
  • build_run_stats merges the per-worker counts into the exit-channel WorkerStats by (task_id, worker_id); registered workers with no thread in the server process (fully external workers) get synthetic entries carrying their block counts.

Semantics notes:

  • Counts are now identical across thread, subprocess-shim, and external workers (the shim child registers with the same worker id as its babysitter thread).
  • Reclaimed attempts (block timeout, dead client) were never valid returns and are no longer counted — slightly more honest than the old thread-loop counter.
  • Failed-but-returned blocks still count, matching the old accounting.
  • A client running an older daisy that never registers still yields correct per-task counts; only its per-worker attribution is absent. (The reverse — this branch's client against an older server — was already a non-goal: server and workers ship together.)
  • Registration doubles as a liveness signal: registering clears any stale closed-connection marker left on an OS-recycled ephemeral port, and disconnecting drops the registration, so recycled ports can't inherit stale identities or get their in-flight blocks falsely reclaimed.

Internal API change: Client::connect and _rs.SyncClient now take a worker_id (both internal; daisy.Client reads it from DAISY_CONTEXT as before). docs/source/design/RUN_STATS.md is updated — the "known gap" section is replaced by the Register design, with daisy.profile_block kept as the future path for per-block CPU/RSS.

Tests (9f6bf9f)

  • tests/test_worker_serialization.py (new): forces the by-value failure mode deterministically with a synthetic module carrying a threading.local + struct.Struct (so coverage doesn't depend on install layout), the daisy-module-global shape, __main__-defined function end-to-end through real subprocess workers, eager-failure guidance, and _PicklableLocal round-trip.
  • tests/test_run_stats.py: the previously pinned tests now run the default subprocess mode and their blocks_processed assertions pass; a new pinned test keeps thread-mode counting covered.
  • Rust: unit tests for RunTally counting and the build_run_stats merge (including synthetic external-worker entries and the unregistered-client fallback); bookkeeper registration lifecycle + recycled-port hygiene; the TCP integration test now asserts externally-connected registered workers appear in run stats.
  • Full suites: cargo test -p daisy-core 46 passed; pytest tests/ 204 passed, 1 xfailed (the intentional daisy.messages documentation xfail).

🤖 Generated with Claude Code

https://claude.ai/code/session_01KxYd8bCjgZLoESSe3smbwF

…lding daisy if necessary utilities are missing
@rhoadesScholar rhoadesScholar self-assigned this Jul 27, 2026
@rhoadesScholar rhoadesScholar changed the title (feat): inform users of necessary additional steps for installing/bui… (feat): Loud fail for missing install tools, and bug fixes Jul 27, 2026
@rhoadesScholar
rhoadesScholar requested a review from pattonw July 27, 2026 23:45
The serializer fix was only covered incidentally (tests that fail only
under editable installs); test_worker_serialization.py now forces the
by-value failure mode deterministically with a synthetic module, and
covers the daisy-module-global shape, the __main__ end-to-end path, the
eager-failure guidance, and _PicklableLocal. Thread-mode block counting
regained a pinned test after the run-stats tests were unpinned to the
default subprocess mode. Rust side: unit tests for RunTally counting and
the build_run_stats merge (including synthetic external-worker entries),
bookkeeper registration lifecycle and recycled-port hygiene, and the TCP
integration test now asserts external registered workers appear in run
stats.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KxYd8bCjgZLoESSe3smbwF
@rhoadesScholar

Copy link
Copy Markdown
Contributor Author

Closing as superseded. This PR bundled three unrelated changes — its own title announced all three, and
the middle commit's message was literally "(fix) assorted bugfixes -- needs review". Reopened as three
single-purpose PRs, each off v2.0 and each with a runnable failing→passing reproduction in its
description. Fresh PRs rather than a force-push so the review history stays readable.

#72 feat(build): fail with actionable instructions when a C compiler or cargo is missing build_wrapper.py, pyproject.toml, .gitignore
#73 fix(workers): pickle modules by reference so subprocess payloads survive editable installs 2 Python modules + tests/test_worker_serialization.py
#74 fix(run-stats): count blocks server-side so blocks_processed is correct in every worker mode the Rust core, the protocol, RUN_STATS.md, tests/test_run_stats.py

They share no files, so they can be reviewed in any order. #74's description notes one ordering caveat
for reproducing its MWE under an editable install.

Two things this PR got wrong that are fixed in the split

1. It broke the docs CI, and I hadn't noticed. Commit 357d040 converted [dependency-groups] to
[project.optional-dependencies] but commented the old table out instead of deleting it, and did not
update the three places that consumed it — .github/workflows/docs.yml (two call sites) and
README.md:52 all still said uv sync --group docs:

$ uv sync --group docs
error: Group `docs` is not defined in the project's `dependency-groups` table

So the build job failed at dependency install on this branch, and #71 inherited it by being stacked
here. #72 does the conversion and the updates together, since they are the same change.

2. The claimed test results did not hold in a clean dev environment. This PR's description says
pytest tests/ gives 204 passed / 1 xfailed. On a fresh uv sync --group dev,
tests/test_worker_serialization.py::test_main_defined_function_end_to_end fails with
ModuleNotFoundError: No module named 'funlib'funlib.geometry is undeclared and reaches the
environment only transitively via lsd-lite in the examples/docs groups. Installing it gives 5
passed. That is a pre-existing repo-wide gap rather than something this PR introduced, so #73 flags it
for a separate one-line fix instead of folding it in, but it will break any pytest CI job the moment one
is added.

Also found while splitting, and worth separate issues

  • tests/test_run_stats.py::test_slowing_workload_reports_positive_slope fails reproducibly on
    v2.0
    (3/3 runs, on trees that touch nothing in run_stats). fix(run-stats): count blocks server-side so blocks_processed is correct in every worker mode #74 fixes it as a side effect.
  • publish.yaml's test-wheel job fails, and release gates on it — so tagging v2.0.0 today would
    build every wheel successfully and then not publish. Pre-existing, fails identically on v2.0.
  • GitHub Pages is on build_type: "legacy" (source gh-pages branch) while docs.yml uses
    artifact-based actions/deploy-pages, which requires build_type: "workflow". So the deploy job can
    never succeed and the published site is still the v1-era build from 2025-03-28 — none of the v2 design
    docs, API reference, MIGRATION.md or tutorials have ever been published. Needs a repo admin to flip the
    setting; happy to file an issue if that's the right route.
  • There is no test or lint CI on the v2.0 line at all — no pytest job and no cargo test job,
    though master still has tests.yaml/ruff.yaml/mypy.yaml. 186 Python tests and 46 Rust tests exist
    and nothing runs them, which is how both items above stayed invisible. dev/REFACTOR.md §1.1 already
    records integration_tcp.rs breaking twice for the same reason.

Happy to open any of those as PRs if useful.

rhoadesScholar pushed a commit that referenced this pull request Aug 3, 2026
Pure `ruff --fix` pass over the Python sources with
--select I001,F401,F811,RUF100,UP035,PYI029,PYI041. No behaviour changes:
import sorting, unused-import removals, duplicate-import dedups, redundant
noqa removals, typing.Callable -> collections.abc.Callable, and two .pyi stub
cleanups (redundant __repr__ declarations; float|int -> float).

Rebased from v2.0_patch onto v2.0 now that #70 is closed in favour of #72/#73/#74.
Regenerated rather than cherry-picked, so the two hunks that only existed via #70
(build_wrapper.py, tests/test_worker_serialization.py) are simply absent.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pattonw added a commit that referenced this pull request Aug 3, 2026
Alternative to PR #70's custom dill Pickler subclass (branch
v2-bugfix-worker-serialization): rather than teaching dill to pickle
modules by reference, use the serializer whose DEFAULT is exactly that.

dill pickles any module outside site-packages by value — entire __dict__
— so a block function that references its own project package as a
global (import mypkg; mypkg.helper()) fails the payload if that package
holds any unpicklable module-level object, even an untouched one.
Reproduced on a normal non-editable install: TypeError: cannot pickle
'_struct.Struct' object. cloudpickle pickles importable modules by
reference and reserves by-value for __main__ — the exact split daisy
needs, since workers replicate the parent's sys.path and remote cluster
workers re-import anyway. Deletes the need for a hand-maintained
dispatch override and for the _PicklableLocal workaround.

Trade-off, documented in the module docstring, MIGRATION.md and
CHANGELOG: cloudpickle refuses threading primitives and write-mode file
handles (directly or via a bound method's self), where dill silently
shipped a lock that synchronized nothing. daisy now raises at submit
time naming both fixes (construct inside the block function, or use
worker_processes=False where locks are real).

This exposed two vacuous tests: test_resources' inline task builders
measured cross-worker concurrency with in-process counters while running
subprocess workers, so the parent observed peak=0 and 'peak <= budget'
asserted nothing. Pinned to thread workers like the module's factory
already was, so they measure again.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@rhoadesScholar
rhoadesScholar deleted the v2.0_patch branch August 3, 2026 22:44
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