v2.0.2 - Improve large experiment view performance and output isolation - #306
Open
guillaume-byte wants to merge 17 commits into
Open
guillaume-byte wants to merge 17 commits into
guillaume-byte wants to merge 17 commits into
Conversation
* feat(agent): vendored OpenCode binary + auto-install/init and container-tunnel support --------------- - opencode_binary.py: Node-free on-demand provisioning of the OpenCode standalone binary (npm-registry tarball via stdlib), managed per-user cache - resolver prefers managed binary; background auto-install on import/start - `weightslab agent init` CLI; agent-config gating with an info hint (no implicit sign-in) - configurable OpenCode bind host + UI trusted-hosts allowlist so the agent works through a container's published port / SSH tunnel - unit tests + CI agent-smoke job Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(agent): Getting Started via `weightslab agent init` + OpenCode env var reference - agent_quickstart: lead with `weightslab agent init` (provisions the Node-free OpenCode binary, then signs in); document --provision-only and the "agent not initialized" info behavior so a new user knows exactly what to do. - configuration: document the OpenCode provisioning/bind env vars — WEIGHTSLAB_OPENCODE_HOST, WEIGHTSLAB_UI_TRUSTED_HOSTS, WEIGHTSLAB_OPENCODE_AUTOINSTALL/AUTODOWNLOAD/VERSION/HOME — incl. the container-behind-a-tunnel setup.
* fix(v2.1): video-gen report media + server-authoritative subview flag - reporting: add a "Generated Media" section (poster thumbnails per media field) so video/image-generation runs show their artifacts instead of an empty report; guard media columns in the Distributions path (no longer mislabelled "no numeric values"); surface a swallowed get_combined_df error as a warning so a broken dataframe isn't silently hidden. - data_service/proto: DataSamplesResponse gains is_subview/view_count/ total_count, stamped from the backend's _is_filtered state on every GetDataSamples, so a fresh client can render the subview warning ribbon with no cached UI state. Regenerated pb2 with grpcio-tools 1.68 (gencode 5.28.1). - tests: report media/distribution-guard coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(v2.1): backend min/max curve decimation so spikes survive 10k->1k get_signal_history_downsampled emitted the earliest-step point per bucket, so a spike between bucket edges (unless separately flagged as marker/note/outlier) was dropped server-side before the browser ever saw it. Now it emits each bucket's min-value AND max-value rows (min/max decimation); the bucket count is halved so the total stays ~max_points. 10k points -> ~1k, with spikes. + test: a non-flagged mid-bucket value spike survives, output stays ~max_points. * Check OS and adapt cmd
test_logger_scale.py deliberately builds multi-million-row DuckDB fixtures to stress-test large-scale queries -- real, by-design heavy work that easily exceeds the per-test timeout on a shared runner. Its own marker registration already said "deselect with -m 'not scale'" but the CI job never actually did, so each run burned its 600s per-test timeout on these instead of skipping them, reading as a hang stuck at the same progress percentage across unrelated pushes.
Examples no longer reach into the package internals. Every
`from weightslab.components.global_monitoring import (guard_training_context,
guard_testing_context)` is gone; the call sites use `wl.guard_training_context` /
`wl.guard_testing_context`, which are the very same singletons (the top-level
names are lazy re-exports of that module, verified by identity).
The Ultralytics trainers now have the same treatment: `WLAwareTrainer`,
`WLAwareSegmentationTrainer`, `WLAwareDataset` and `WLAwareSegmentationDataset`
are re-exported from the package root, so a YOLO script reads
`trainer=wl.WLAwareTrainer` with no deep import. They go through the lazy export
map rather than a plain import because `ultralytics` is an optional extra --
importing it eagerly would break `import weightslab` for everyone without it.
`__getattr__` now turns that missing dependency into an actionable error
("needs the optional 'ultralytics' package: pip install 'weightslab[ultralytics]'")
instead of a bare ModuleNotFoundError, and still raises AttributeError for names
that genuinely do not exist.
Notebooks are stripped of outputs and execution counts (23 notebooks verified
clean). Edits are byte-surgical rather than an nbformat round-trip: these files
carry different JSON indents (1 for Jupyter, 2 for Colab-saved) and CRLF, so a
round-trip would have reformatted whole files and buried the real change. Every
cell's source was diffed against HEAD to confirm nothing else moved.
Docs and code samples follow the same path (`wl.guard_*`, `wl.WLAwareTrainer`),
including the README wandb-migration diff and both AGENTS.md files so the next
agent does not reintroduce the deep imports. Prose mentions of the bare names
are left as they are -- those names remain importable from the package, so the
text stays correct.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
# Conflicts: # docs/examples/ultralytics/detection.rst
…#305) The studio notebook runs an embedded ipykernel inside the trainer's own process, and IPKernelApp.initialize() swaps sys.stdout/sys.stderr process-wide for an OutStream that publishes every write to iopub. So the training loop's tqdm bar -- a different thread, writing continuously -- surfaced in whatever cell was last executed ("Training: 193497 steps ... train_loss=1.4612" in a cell that never asked for it). The legacy in-process kernel leaked the same way: contextlib.redirect_stdout is process-global too. Route the streams per write instead of per process: * _ThreadRoutedStream wraps ipykernel's OutStreams -- the thread currently running a cell reaches the kernel stream, every other thread (at any time, including while the kernel is idle) gets the console stream the process had before the kernel existed. Ownership comes from the pre_execute/post_execute hooks, which run on the real execution thread, so nothing here assumes which thread ipykernel picked for the shell channel. * capture_fd_output=False, or ipykernel's fd 1/2 pipe would re-capture exactly the writes that were just routed back to the terminal. The cost is that output written straight to the fds by C extensions no longer reaches the notebook -- for an in-process kernel sharing a terminal with the trainer, that is the better trade. * _LiveStream (legacy kernel) got the same thread check, falling back to the pre-redirect stream so those writes still reach the console. Two tests: one in the shared contract class, so both kernels are held to it (a background thread's output must not appear in a cell, while the cell's own print still does), and a legacy-only one proving the other thread's writes reach the console rather than being dropped. Known consequence: output from a thread a cell itself spawns now goes to the terminal, not the cell. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…es (#303) * docs(perf): register O(data) operations blocking 100GB+ interactivity Storage and serving paths whose cost scales with dataset size rather than with what changed. Storage findings re-verified against dev; two serving costs noted as already fixed upstream so they are not re-claimed as wins. No code changes - baseline and measurement protocol only. * docs(perf): triage every _slowUpdateInternals call site 16 of 18 sites only need fresh values for dirty rows (O(change)); only first build and schema change need a full reconstruction. Records why ApplyDataQuery filter paths stay on the rebuild (_is_filtered semantics) and why a no-client benchmark cannot show the difference. * perf(interactivity): make view refresh and ledger writes O(change) Every path that kept the served view in sync ran O(dataset): a full materialized-view rebuild on each signal tick, and a read-modify-append of the whole H5 table per upsert. At 3.96M rows that meant a 670s startup and lock holds long enough that the UI looked hung while training. Three changes, each turning a whole-dataset pass into a per-change one: data_service: differential view refresh (_fastUpdateInternals). The materialized view only ever recomputes index-derived state, so a value-only delta can be written straight into the existing view through a sample_id -> row-position map instead of rebuilding it. Falls back to the full path on any structural change (unknown sample_id, missing pos map, backlog over max_dirty), so correctness never depends on the fast path being right about a schema change. 9 call sites routed here; the 10 that genuinely change shape still force a rebuild. Off with WL_FAST_VIEW=0. The sort path is restructured to do its work off-lock: ops and the pos-map rebuild both run on a shallow copy, and only the pointer swap happens under the lock. Skipping the pos-map rebuild after a sort would have been a data corruption bug -- sorting reorders the view, so stale positions send differential writes to the wrong rows. h5_dataframe_store: in-place row updates via modify_coordinates, with a cached sample_id -> coordinate map (stable row positions come free from the no-row-loss invariant). PyTables cannot invalidate a column index during modify_coordinates, so _try_inplace refuses indexed tables and the caller falls back to the append path. Index construction is also split from storage layout: data_columns=True keeps the on-disk layout queryable while index=False keeps the flush path from rebuilding an index no hot-path read uses (92.7s -> 6.9s per upsert at 4M rows). dataframe_manager: replaces the per-row iterrows() scans that dominated startup with column-wise vectorised passes, and adds the dirty-row/source-row accessors the differential refresh needs. Also fixes an unrelated thumbnail bug in trainer_tools.process_sample: it unpacked exactly 3 values from _getitem_raw, whose contract is (data, id, target, *metadata). Any dataset implementing get_items() with metadata raised "too many values to unpack" and every cell in the grid came back with no image. Now unpacked positionally. Measured on 3.96M-row UltraEdit, A10G: startup 670s -> 250s H5 upsert (24 rows) 127.9s -> ~16ms snapshot flush 338s -> 0 samples max lock hold 129,440ms -> none over 1s throughput under UI 13% -> 45-51% of idle The residual loss under load is CPU/GIL contention (8 vCPUs shared by 6 dataloader workers, training, and image encode), not lock waiting. Known gaps, deliberately left for review: - ensure_index() has no caller yet, and conflicts with _try_inplace, which refuses indexed tables. It documents the deliberate-index path but is dead code as committed. - _POSMAP_CACHE is class-level and never evicts (~400-600MB at 4M rows). - Three ApplyDataQuery sites still force a full rebuild pending a decision on _is_filtered semantics. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(view): stop the served view silently diverging from the ledger The ledger was always correct; the view readers see was not, and every failure mode reported itself as success. On a 3.96M-sample run the UI showed ~2k samples with loss data at 34k steps, and toggling image modalities showed the source image twice. View correctness: * Address differential-sync rows by the SAMPLE_ID index level. The view is indexed (origin, sample_id), so get_level_values(0) returned origin and its intersection with the dirty sample_ids was always empty -- the sync wrote nothing and returned True, which suppressed the rebuild that would have repaired it. Positions now come from Index.get_indexer (cached hash engine), so it stays vectorised. * Rebuild when the ledger gains columns the view lacks. Per-sample signal columns are created on their first write, so on a fresh ledger the view predates them and could never gain them: sorting and histogramming failed with "column not in view" and last_seen served -1 forever. Checked before the dirty-set drain, since a schema gain is independent of dirty rows. * Keep the view-dirty backlog until a rebuild actually lands. It was discarded on overflow assuming the caller would rebuild, but the force path returns early on a contended lock -- those ids were then lost with nothing left to re-mark them. Cleared at the atomic view swap instead. * Log view-build failures as errors. They were swallowed at debug level and returned the previous view, so a broken build was indistinguishable from "no new data". Named image views: * Probe extra_images() on the unwrapped dataset -- WL's tracking wrapper does not forward it, so every named view was silently dropped. * Honour stats_to_retrieve for image views, but still advertise filtered-out views with an empty thumbnail so their toggles do not vanish from the panel. Cost: * Loss-shape autotagging runs on its own interval (WL_LOSS_SHAPE_INTERVAL_SECONDS, default 60s) instead of the 2s flush tick, where each pass cost ~990ms of GIL-held pandas work. * Signal-DAG history reads a bounded in-memory tail (WL_HISTORY_TAIL, default 16) instead of scanning per_sample -- 140ms per step at 20M rows, growing without bound. Neither an index nor a rewritten IN clause helped (1.1x/1.4x). * Skip array normalisation for columns the H5 write excludes: with predictions off it rasterised via get_mask, which reads the source image, for data that is never persisted. * Close inherited HDF5 fds in forked dataloader workers; they made HDF5 refuse the parent's read-write open and killed ledger persistence for 12 hours. Measured on the UltraEdit harness (859M params, batch 24, A10G) against an identical run with weightslab stubbed out: 1574ms -> ~1290ms/step versus a 1171ms baseline, i.e. +34% -> ~+10%. optrace.py is included: the @traced/hit markers the other files import are what located the sample_id level bug. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(histogram): bin over rows that carry a value, not every row in the view The numeric path cut bin boundaries by row position across the WHOLE view and dropped non-finite values only afterwards, so every bucket spanned len(view)/max_bins rows regardless of the data. On a 3,963,189-row view with 512 bins that is 7,740 rows per bucket -- so a column where only ~33k samples carry a value (any signal early in a run) collapsed into the first four buckets, and the remaining 500 sliced empty space into 1-6 sample slivers. Visible as four fat bars followed by a long tail of random-looking spikes, with the same 7740/7741 counts appearing on unrelated columns because the number came from the row count, not the data. These bars are a search surface over the loss landscape: each should be a click-target holding a comparable number of samples. Mask first, then cut equal-population boundaries over the finite subset. Row ORDER is untouched, so this is still "bin the current view by row order" -- it just stops counting rows that have nothing to show. Also fixes the per-(origin, discarded) sub-bars, which were grouped by the same positional bins. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(ledger): make the NB_SEEN lookup O(batch), and regenerate the protos Three things, all needed to make the branch runnable on top of dev at 4M rows. 1. Regenerated experiment_service_pb2{,_grpc}.py dev's .proto declares AnnotationExportFormat / EXPORT_FORMAT_CVAT but the committed gencode predates it, so a clean checkout of dev does not import at all: AttributeError: module 'weightslab.proto.experiment_service_pb2' has no attribute 'EXPORT_FORMAT_CVAT' Regenerated with grpcio-tools 1.68.1 (protoc 5.28.1), matching the runtime version already pinned in the file, so the gencode major does not move. 2. Cache the level-0 index for sample-id coercion _coerce_sample_id_for_index() called index.get_level_values(0) on every invocation. That materialises a fresh Index over all rows, and a fresh Index carries a fresh hash engine, so each `sid in level_0_values` paid a full engine build -- twice per sample when the int probe missed. enqueue_batch does that per sample, 24x a step: training sat at 0 iterations with the main thread pinned at 100% CPU inside pandas __contains__ (py-spy: active+gil). Cached on the index object's identity. pandas Index is immutable, so any reindex or rebuild yields a new object and invalidates it; membership semantics are unchanged, the engine is simply reused. 3. Positional NB_SEEN lookup get_sample_column_values() then still materialised both index levels, ran isin over every row and copied a boolean-masked frame -- a full pass plus a copy over 3.96M rows to read 24 integers, ~1.2s/step (signals 6ms -> 1230ms, total 1290ms -> 2500ms). The wanted rows are exactly (sample_id, 0), so resolve their positions with Index.get_indexer instead. Falls back to the original scan when the index is not unique. Measured on the UltraEdit harness (859M params, batch 24, A10G, 3.96M samples): signals 1230ms -> 28-41ms total 2500ms -> 1310-1338ms (1171ms with weightslab stubbed out) NB_SEEN now actually increments (it was stuck at 0 before dev's fix), verified against the ledger: rows with nb_seen>0 equals rows with last_seen>=0. The UI contract suite passes 21/21 on this build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(data_service): bin the numeric histogram over the whole view again Each bar must cover total_rows / max_bins samples so the chart carries density: a column that is only 0.2% populated should show a few filled bars and the rest empty. Binning over just the rows that carry a value made the chart look equally full at any coverage, which reads as "every sample already has a loss". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * proto: regenerate with package-relative imports after the dev merge dev checks in generated code that does a flat 'import experiment_service_pb2', which only resolves if weightslab/proto is itself on sys.path. Imported as a package -- which is how the trainer loads it -- startup dies with ModuleNotFoundError. Regenerated from the merged .proto at the repo root so dev's new RPCs are kept and the import is package-relative again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(logger): import deque alongside defaultdict The merge re-applied our in-memory history tail onto dev logger.py, which imports only defaultdict. Every per-sample write then raised NameError inside _stage_sample_row. The caller swallows per-signal exceptions, so nothing crashed: the tail just stayed empty, sig/loss_debiased failed every step, and loss_shape had no history to classify. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(shapes): keep the label cache on top of dev write_signal_shapes dev rewrite keeps the O(change) read and adds exp_hash scoping, both kept. What it dropped is the label cache, which two behaviours depended on: - an incremental pass still returns a distribution over the WHOLE dataset, not just the samples it happened to touch; - a sample whose label did not change is not re-written to the ledger. Both are asserted by e2e_autotag (distribution_covers_dataset, incremental_writes_bounded), which failed on the merge until this went back. The test also moves to dev parameter name, sample_ids. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(signals): restore inputs= on the batched subscribe_to path On the subscribe_to path BatchSignalContext was built without inputs=, so b.inputs was {} and any signal declaring inputs=[...] raised KeyError on every call. sig/loss_debiased does exactly that: it failed 12,079 times in one five hour run -- once per step -- and because wrappered_fwd swallows per-signal exceptions nothing crashed, the column just silently never got values. We had already fixed this; taking dev src.py whole during the merge reverted it, since dev never carried the fix. Same class as the deque import and the label cache: dev has no equivalent, so a wholesale take drops it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Remove optrace tracing from weightslab Drops the optrace module and every call site: 64 @Traced decorators, 13 hit() markers and 5 imports across the data stores, the dataframe manager and the two services. Pure deletion -- 437 lines out, 0 in. Every hit() was verified to be a bare statement rather than an expression, so removing it cannot change a value, and removal was parenthesis-balanced because several spanned three lines. Each file is compiled after editing, which is what would catch a removal that left an empty block. The tracing was built to find where interactivity time went on a 100GB dataset. It has served that purpose: the O(change) view sync, the O(batch) NB_SEEN lookup and the flush accounting all came out of it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ci: fix the code-quality and gRPC-test failures on this branch Three fixes, one per failing check. ruff F841, trainer_tools.process_sample: the positional unpack of _getitem_raw bound _res[1] to idx, which nothing reads -- the function returns sid. Dropped. ruff F401, examples/.../wl-video-generation/utils/data.py: unused `os` import. Pre-existing on dev and untouched by this branch; it only surfaces here because the lint step appends ./weightslab to the changed-file list, so ruff scans the whole package on any PR that touches it. Removing it is what unblocks the gate. AttributeError in tests/gRPC/test_grpc_user_actions.py: _fastUpdateInternals duck-types take_view_dirty and get_source_rows on the df manager. Both are new on this branch, so _FakeDFManager -- and any third-party manager -- raised AttributeError instead of taking the fallback. Guarded: a manager without dirty tracking cannot serve a delta, which is the same "structural change" case the method already falls back on, and the caller then runs _slowUpdateInternals exactly as before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RHV8zUd5aCtHtQourmwUJC * fix(agent): one shared OpenCode model for the studio, the CLI and the backend The backend answered on whatever model it resolved at start-up, and nothing could move it afterwards: a model picked in the studio was ignored, `agent model X` reported "Model switched to <the old one>", and `agent status` named a model that was no longer in use. OpenCode's own config (GET /config) is now the single shared choice, with: 1. OPENCODE_MODEL -- a hard pin, for automation. Never overridden; if the studio disagrees, `agent status` says so. 2. GET /config's model -- the live shared choice, RE-READ BEFORE EVERY TURN rather than latched at start-up. 3. agent_config.yaml's opencode_model -- a SEED: used when nothing has been chosen yet, and published so the studio shows it. It used to pin, so a run started after picking a model in the studio went back to the yaml value. 4. opencode/big-pickle -- the built-in default (was opencode/deepseek-v4-flash-free), also published. Whoever chooses last wins, and both surfaces follow. publish_model() writes PATCH /global/config (falling back to /config) and CONFIRMS by reading back: the workspace route answers 200 for a write it drops. An explicit switch (`agent model`, `agent init --model`, the RPC) is published rather than overwritten by the shared-config read, and current_model() re-resolves so `agent status` reports the model the NEXT query will use. The start-up banner now names the model actually in use and where it came from instead of printing "(server default)" whenever nothing was pinned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(cli): hand the experiment directory to runs started from another terminal `weightslab start` establishes the experiment directory and exports WEIGHTSLAB_ROOT_LOG_DIR -- into its OWN process only. A training run launched from a second terminal (or by `weightslab start example --seg`) is a different process tree and never saw it: it fell through to tempfile.mkdtemp(), so the run wrote reports/, notebooks/ and checkpoints into %TEMP%\tmpXXXXXXXX while the UI listed an empty reports/ from the directory it had established. That is the "right-click Generate report lists nothing, yet I generated reports" bug. weightslab/utils/active_experiment.py records the directory in a small per-user marker (~/.weightslab/active_experiment.json, WEIGHTSLAB_STATE_DIR to relocate), with two independent sections: `ui` (what `weightslab start` established) and `backend` (where training ACTUALLY resolved, whatever the route). Both writes are best-effort and every read validates the directory still exists. * root_log_dir resolution gains a step: explicit config > env > the recorded `ui` directory > temp dir. The temp-dir case now warns loudly that the UI will not find the run's files. * `weightslab start example` passes the recorded directory to the child, and says which directory it is using. Anything already set in the shell wins. * The UI's reports/notebooks/agent listings follow a LIVE backend's own recorded directory, so they stay right even when training was pointed elsewhere by a config file. Only a live one counts: the marker outlives the process that wrote it, and a finished run must not hijack the listing of a UI that was given its own directory. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(ui): proxy the shared agent model same-origin The studio picker wrote OpenCode's shared model directly from the browser, which only works while OpenCode's --cors allowlist contains the page's exact origin. A LAN address, a tunnel hostname, or an `opencode serve` started by hand with no --cors all make that cross-origin PATCH fail its preflight, so the pick never reached OpenCode -- and therefore never reached weightslab's backend, which reads the same field to choose the model for its own queries. GET/POST /agent-server/model proxy it through this server instead: same-origin for the page, plain HTTP to OpenCode on the machine they share. The write goes to the global scope first and is confirmed by reading /config back, because the workspace route answers 200 for a write it drops. A transport failure or an error status is reported as ok:false rather than "nothing configured", so the page never shows its own default over the model actually in use. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(data): stop a missing flag reading as a set one (phantom discards) On a detection/segmentation run, sample after sample greyed out as "discarded" in the studio as the model worked through the dataset -- while the dataframe said nothing was discarded. It came down to one line of Python semantics: bool(float("nan")) is True. The chain: 1. the trainer touches a sample -> its rows go dirty; 2. _fastUpdateInternals syncs the trainer-owned columns (signals*, last_seen, discarded, prediction, target) from the ledger into the served view. It collapsed the per-annotation rows with duplicated(keep="last"), keeping the LAST annotation row -- whose sample-level columns are NaN, because the real values live on the canonical row (annotation_id == 0, which is what the view itself is built from); 3. NaN therefore landed in the view's `discarded` (and in `prediction` / `target`, and `last_seen` went stale); 4. GetDataSamples served it as "1" if bool(value) else "0". Only annotation-expanded ledgers, only samples training had touched. Fixed in three places, plus the two siblings of the same bug found while auditing the function: * the differential sync now takes the canonical annotation_id == 0 row (falling back to the first occurrence), matching how the view is built; * is_set_flag / set_flag_mask replace bool() / astype(bool) wherever a nullable flag is read: the `discarded` rendering flag, the boolean tag:* columns in the metadata response -- where astype(bool) turned the NaN of every UNtagged sample into True, i.e. every sample wearing every tag -- and the histogram's per-(origin, discarded) split. They also read the strings "True"/"False" correctly, which a column that has been through the H5 store (categorical) can hold, and where bool("False") is True as well; * the sync's position lookup no longer searches the view's sample_id level, which raises InvalidIndexError as soon as one sample_id appears under two origins -- the very thing the view's (origin, sample_id) index exists to allow. It failed on every call there and silently fell back to the full rebuild. 13 tests reproduce the chain and each sibling; every one of them fails against the previous code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(cli): only a running `weightslab start` hands its experiment dir over The marker outlives the process that wrote it, so a directory recorded by a UI that had since exited redirected unrelated runs. It bit this repo's own suite: tests/gRPC/test_grpc_tag_operations.py resolved its root_log_dir into a previous session's experiment, found the segmentation example's config and checkpoints there, and errored in setUp with an unrelated config. * the handoff now reads live_ui_experiment_dir(), which requires the recording process to still be alive -- which is what "the UI is up over there, put this run in its experiment" actually means; * tests/conftest.py points WEIGHTSLAB_STATE_DIR at a throwaway directory for the whole session, so no test ever reads (or writes) the developer's own WeightsLab state, whatever a future one happens to resolve. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(data): default `discarded` instead of leaving it NaN SampleStats.DEFAULTS documents `discarded` as False, directly under the comment "None are not accepted by PD H5 storage" -- so a NaN in it was already a broken contract, and it is where the phantom-discard bug started. The existing normalisation could not catch it: it only visits columns an upsert ADDS (`missing_cols`), and only when the incoming slice's dtype is already bool -- which it is not exactly when the slice carries missing values. So a sample registered without the flag, and every per-annotation row (sample-level values live on annotation 0), kept a NaN, and bool(NaN) is True. _fill_documented_flag_defaults() gives every column with a boolean default in SampleStats.DEFAULTS its default after each upsert. An isna().any() short-circuit per column means the common case touches no rows; a categorical column (what the H5 store hands back) is widened first, since fillna on a Categorical raises for a value outside its categories. Deliberately NOT applied to tag:* columns: for a boolean tag, NaN and False mean the same thing and NaN costs nothing, and for a categorical tag NaN means "unset", which is not a default at all. The read side (set_flag_mask) already treats both as not-set. Belt and braces with 4819483: the flag is defaulted at the source AND a NaN that reaches a reader anyway is read as not-set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Alexandru Rotaru <rotarualexandruandrei94@gmail.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Guillaume <guillaume@graybx.com>
Both pairs were test bugs, not defects in the code under test.
tests/test_opencode_binary.py -- resolve_opencode_argv() returns str(Path), so
the expected value has to be spelled the same way: str(Path("/mgd/opencode"))
is "/mgd/opencode" on POSIX and "\mgd\opencode" on Windows. The POSIX form was
hard-coded, so `test_managed_present_wins` and `test_download_when_no_path`
failed on Windows only. Compare against str(Path(...)).
tests/general/test_four_way_standalone.py -- `set_hp` refuses to guess when the
process holds more than one hyperparam set ("Multiple hyperparam sets present;
provide hp_name explicitly"), and the ledger is global: the sets registered by
the other levels in this file, and by any module that ran earlier in the same
process, are still there. So the two set_hp tests passed or failed depending on
what had run before them. They now name their set via resolve_hp_name(),
exactly as test_hp_lists_and_shows in the same file already does for `hp` --
and which its own comment explains was added for this reason.
Full suite as CI runs it (pytest ./tests -m "not scale"): 1901 passed,
145 skipped, 17 deselected.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
test_grpc_serve_honors_explicit_port_without_force_parameters timed out on commit ac40533's PR run (35738307562) and passed on its push run (35738302340) -- same commit, same workflow. It was never deadlocked. serving_thread_callback logs its way through the bind, and in a full-suite run each of those calls takes SECONDS: 00:35:29.215 [gRPC] Thread callback started 00:35:39.294 [gRPC] Creating ThreadPoolExecutor <- +10.1s 00:35:44.893 [gRPC] Server object created <- +5.6s Three log lines, 15 seconds. Every assertion would have passed; the test just never reached them before _TimeoutMixin fired -- hence a timeout rather than a failure, and a green rerun every time. The contention comes from threads earlier tests start and never stop (the embedded ipykernel's tornado/zmq loop, dataframe_manager's flush threads and their DEBUG chatter): by the time pytest reaches tests/trainer/ they hold the logging lock often enough to starve it. tests/trainer/services on its own passes -- 386 passed. - stub the module logger in _run_grpc_serve_capturing_bind. These tests assert on add_insecure_port and have no interest in log output, so the fix is to stop depending on real logging rather than to outwait it. - _TimeoutMixin built its exc_info with a None traceback, so pytest raised "'NoneType' object is not iterable" while rendering and fell back to "Incompatible Exception Representation" -- the report named no location at all. Raise and catch instead, for a real traceback. - 30s -> 60s: the cap guards against a stuck thread, it shouldn't double as a performance assertion. This alone did NOT fix the hang. - conftest: keep the real ResourceMonitor out of the suite. grpc_serve ends by calling start_resource_monitor_from_config() and nothing stubbed it, so the first such test left a process-wide singleton sampling CPU/memory/disk/ network/GPU for the rest of the session. One of the leaked threads above. Full suite: 1847 passed, 145 skipped, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI died on the way out of a run whose tests had all passed:
Fatal Python error: _enter_buffered_busy: could not acquire lock for
<_io.BufferedWriter name='<stderr>'> at interpreter shutdown, possibly
due to daemon threads
... Aborted (core dumped)
The embedded Jupyter kernel runs as a daemon thread on app.start() -- a
tornado/zmq loop that never returns -- and had no shutdown path at all. So it
was still live when CPython began finalizing, and the shutdown raced itself:
the interpreter closes the zmq sockets, tornado's zmqstream notices ("Got
events for stream ... attached to closed socket") and calls gen_log.warning,
and logging writes into a stderr buffer whose lock is already being torn down.
That aborts the process; the job then fails on the exit code even though the
suite passed.
Not test-only, which is why this is here and not in tests/conftest.py: any
script that enables the notebook takes the same risk at exit.
Stop the kernel from an atexit hook, which runs BEFORE finalization, so
app.start() returns and the thread exits while logging still works.
The loop to stop is not the obvious one. _run_embedded_kernel sets up an
asyncio loop for its thread, but ipykernel builds its own AsyncIOMainLoop
under app.io_loop and blocks on that instead -- stopping ours is a no-op
(measured: our loop ...692880 vs app's ...181904, thread still alive after a
10s join). Go through app.io_loop, whose add_callback is thread-safe, and join
with a bounded timeout so a stubborn kernel can't hang the process on the way
out.
Verified: thread alive -> not alive across the hook; repro exits 0 with zero
"Fatal Python error". Note the abort does not reproduce on Windows -- it is a
platform- and timing-dependent finalization race -- so what is proven locally
is the mechanism, not the end-to-end CI symptom.
Full suite unchanged: 1847 passed, 145 skipped, 0 failed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This branch was successfully deployed
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
2 participants
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.
Summary
Validated on the UltraEdit InstructPix2Pix harness with 3,959,093 samples, 859M parameters, batch size 24, on A10G.
Companion UI work:
weights_studio#151.