fix(streaming): stop layer discovery on IndexError too - #13
agourakis82 wants to merge 2 commits into
Conversation
install_streaming_experts(num_layers=None) counts layers by probing spec.block_of(model, n) with increasing n until it fails, but it only catches AttributeError. Layers live in a list (MoESpec.block_of indexes "layers are plain lists in most families"), so running off the end raises IndexError and the call crashes instead of returning. The shipped engines always pass num_layers, which is why this never showed. Adds test_install_discovers_layer_count: a three-layer list-based model (one MoE layer, two dense) installed with num_layers=None. It raised IndexError before this change; now it returns [twin, None, None] and the twin is swapped into the MoE block. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0146Hf9MXz9hHfRnq3MNXXWX
ahrazzle
left a comment
There was a problem hiding this comment.
Review of PR #13
Verification
The crash on main is real at src/edge0/moe/spec.py:108. The new test fails on base, passes on head, and turns red again when the one-line fix is reverted.
Suite results:
- PR head: 61 passed, 1 skipped
- Merged into current main: 66 passed, 1 skipped
Coverage nits
-
The test exercises discovery stopping at the end of a list, but does not reach the AttributeError branch the widened except preserves.
-
Widening the except to (AttributeError, IndexError) can mask an IndexError raised by an inner numeric segment of block_path. No shipped spec reaches this case.
CI gap
The PR branch ran no checks. Full suite passes on the branch and on merge.
Automated posting by agentic team with human oversight.
…iscovery
Addresses two coverage nits from the PR review of the previous commit:
- The AttributeError half of the widened except was untested: added
test_layer_exists_stops_on_attribute_error, using an attribute-based
layer container (no list, no __getitem__) to reach it independently
of the IndexError case.
- Catching (AttributeError, IndexError) around the whole block_of call
could mask an IndexError raised by a different, non-layer-index
numeric segment further down block_path (e.g. a fixed expert-slot
index) — a real bug, not end-of-list. No shipped spec hits this, but
it would have silently under-counted layers instead of surfacing the
break.
Fixes it by adding MoESpec.layer_exists(model, layer), which resolves
block_path segment-by-segment and only treats an AttributeError/
IndexError as "past the last layer" when it comes from the segment
templated by {layer} itself; any other segment's error propagates.
install_streaming_experts's discovery loop now calls this instead of
wrapping spec.block_of directly. test_layer_exists_reraises_unrelated_index_error
covers the propagation case.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Addressed both coverage nits in d217844:
Full suite: 63 passed, 1 skipped (2 new tests added, no regressions). |
There was a problem hiding this comment.
@agourakis82 Both coverage nits from the first pass are addressed:
-
AttributeError branch now tested.
test_layer_exists_stops_on_attribute_errorconstructs a model whose layer containers use attribute access (SimpleNamespace) rather than indexing. Callingspec.layer_exists(model, 2)runs past the last defined attribute and resolves to the False return, exercising the AttributeError half independently of the IndexError case. -
Unrelated IndexError no longer masked.
layer_existschecks whether the caught exception originated at the{layer}-indexed position before treating it as "past the last layer." Errors from other segments alongblock_pathare re-raised.test_layer_exists_reraises_unrelated_index_errorverifies this with a fixed trailing index (experts.9) that fires before the layer segment would ever run off the end.
Design note: splitting the exception handler inside the traversal loop keeps both paths reachable from the single while spec.layer_exists(...) call in install_streaming_experts. These new tests cover the list container, attribute container, and cross-segment propagation cases.
Automated posting by agentic team with human oversight.
install_streaming_experts(num_layers=None)counts layers by probingspec.block_of(model, n)with increasingnuntil it fails, but it only catchesAttributeError. Layers live in a list (MoESpec.block_ofindexes them: "layers are plain lists in most families"), so running off the end raisesIndexErrorand the call crashes instead of returning. The shipped engines always passnum_layers, which is why it never showed up.Fix: stop on
IndexErrortoo (one line instreaming/install.py).Test:
test_install_discovers_layer_countinstalls into a three-layer list-based model (one MoE layer, two dense) withnum_layers=None. Before:IndexError: list index out of rangefromspec.py:108. After: returns[twin, None, None], with the twin swapped into the MoE block. Full suite passes (pytest, mlx 0.30.6, Apple M5 Max).🤖 Generated with Claude Code