Skip to content

Fix the debugging searches that pruned work they never checked - #85

Merged
cymbalrush merged 1 commit into
apple:mainfrom
cymbalrush:feat/compute-plan
Sep 8, 2026
Merged

cymbalrush merged 1 commit into
apple:mainfrom
cymbalrush:feat/compute-plan

Conversation

@cymbalrush

@cymbalrush cymbalrush commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Search narrowing (search_strategy, validator, comparator):

  • _depth_range was computed and logged but never consulted when choosing what to yield, so bisection visited the whole graph at one model execution per side per batch. Both _group_unchecked_nodes_by_depth and _get_next_batch_from_current_level now filter on it.
  • The lower bound could advance past unchecked siblings when a level was larger than batch_size; the guard now tests <= the passed depth.
  • Status.EXCLUDED was projected to UNKNOWN, which strategies narrow on, so a graph opening with placeholders and views narrowed to depth 0 before a single value was compared. Adds ValidationResult.SKIPPED, which is neither evidence of a fault nor evidence against one.

FX graph diff (graph_diff, graph_match):

  • Only top-level fx.Node args were followed, so aten.cat/aten.stack got no incoming edges and their inputs looked unused. Arguments are now flattened through lists, tuples and dicts, and kwargs are wired by name.
  • An FX node's identity was its op and target alone -- _attr_digest reads an ir_object an FX node does not have -- so cat(dim=0) and cat(dim=1) hashed identically and diffed as isomorphic. Constant args and the exported result type now feed the digest.

Crashes:

  • ComputeDevice.__missing__ is the dict hook, never called by Enum, so an unrecognised residency raised instead of degrading to UNKNOWN.
  • _body_counts recorded a call for a callee with no coreai.graph, and _histogram then raised KeyError on the whole histogram.
  • Benchmarker: interval id 0 was both the "not timed" sentinel and a real id; the sentinel now sits outside the counter's range and is rejected before lookup. _state transitions moved under the callback lock.

@cymbalrush
cymbalrush force-pushed the feat/compute-plan branch 2 times, most recently from be16cb2 to 9a0d480 Compare September 8, 2026 03:46
Six related defects in the debugging package, all of which made a tool
report a clean or unchanged result over work it had not done.

Search narrowing (search_strategy, validator, comparator):
  * `_depth_range` was computed and logged but never consulted when
    choosing what to yield, so bisection visited the whole graph at one
    model execution per side per batch. Both `_group_unchecked_nodes_by_depth`
    and `_get_next_batch_from_current_level` now filter on it.
  * The lower bound could advance past unchecked siblings when a level was
    larger than `batch_size`; the guard now tests `<=` the passed depth.
  * `Status.EXCLUDED` was projected to UNKNOWN, which strategies narrow on,
    so a graph opening with placeholders and views narrowed to depth 0
    before a single value was compared. Adds `ValidationResult.SKIPPED`,
    which is neither evidence of a fault nor evidence against one.

FX graph diff (graph_diff, graph_match):
  * Only top-level `fx.Node` args were followed, so `aten.cat`/`aten.stack`
    got no incoming edges and their inputs looked unused. Arguments are now
    flattened through lists, tuples and dicts, and kwargs are wired by name.
  * An FX node's identity was its op and target alone -- `_attr_digest`
    reads an `ir_object` an FX node does not have -- so `cat(dim=0)` and
    `cat(dim=1)` hashed identically and diffed as isomorphic. Constant args
    and the exported result type now feed the digest.

Crashes:
  * `ComputeDevice.__missing__` is the dict hook, never called by Enum, so
    an unrecognised residency raised instead of degrading to UNKNOWN.
  * `_body_counts` recorded a call for a callee with no `coreai.graph`,
    and `_histogram` then raised KeyError on the whole histogram.
  * Benchmarker: interval id 0 was both the "not timed" sentinel and a real
    id; the sentinel now sits outside the counter's range and is rejected
    before lookup. `_state` transitions moved under the callback lock.

Tests cover each behaviour change. Pre-existing failures in
test_benchmarker (module timings) and test_intermediates are unaffected.
@cymbalrush
cymbalrush merged commit a89a50c into apple:main Sep 8, 2026
2 checks passed
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.

2 participants