[u7k4rs6]

if you pass an ETRecord object to the Inspector instead of a file path, debug handles never resolve. the object's maps have int keys but _gen_resolve_debug_handles looks them up as strings (what you get after save/parse), so every event gets debug_handles=None and no error.

now the maps go through a json round trip in _consume_etrecord so both cases look the same. added a test comparing the object and the saved file, including delegated events.

cc @Gasoonjia @nil-is-all

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/23692

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 7dfd390 with merge base 03b4623 ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[u7k4rs6]

@pytorchbot label "release notes: devtools"

[nil-is-all]

@u7k4rs6 could you update the branch and ping for review?

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[u7k4rs6]

thanks @nil-is-all, updated. @Gasoonjia ready for review when you get a chance.

[nil-is-all]

@claude review

[claude]

Claude finished @nil-is-all's task in 4m 34s —— View job

---

Delegated models still aren't handled: only the outer instruction-id keys get converted to str, so an ETRecord object from a delegated model can now crash Inspector.__init__ with a TypeError where it used to fail silently. Requesting changes.

Detailed review

PR Review: #23692

Summary

The PR converts the instruction-id keys of the ETRecord debug-handle, delegate and num-outs maps to str in Inspector._consume_etrecord, so that passing an ETRecord object resolves the same debug handles as passing a saved path. It works for non-delegated events. For delegated models the conversion is incomplete, and it opens a new crash path that the test doesn't cover.

Code Quality

• Nested delegate_map keys stay int, and this can now crash the Inspector (devtools/inspector/_inspector.py:1144-1148, effect at :997-999)
  • What the fix misses: with_str_keys only converts the outer instruction-id keys. Each DelegateMetadata["delegate_map"] keeps its original keys, which are ints from DelegateMappingBuilder on the object path. After a JSON round trip they would be strs.
  • Before the PR: the outer lookup always missed for ETRecord objects, so delegated events fell back to the outer handles plus a warning, and lines 984-999 never ran.
  • After the PR: those lines run. When the direct lookup at :988 misses (an id that isn't in the map, or a str id against int keys), the fallback loop runs if key in str(delegate_debug_id) with an int key. That raises TypeError: 'in <string>' requires string as left operand, not int from inside Inspector.__init__.
  • Even without a crash: the object path gets tuple values from an exact int match. The file path gets list values from the str lookup and the substring loop. So the "same result as the saved file" goal doesn't hold for delegated models.
  • Fix: normalize the nested keys too. json.loads(json.dumps(...)) reproduces the file shape exactly at every level and would replace the nested helper. Alternatively, fix it at the source (next bullet).
  • All three reviewers flagged this. I confirmed the code path by reading it. How often real backends produce a missing or mismatched delegate id is not verified.
  • Fix this →
• The root cause is patched in one consumer instead of in ETRecord (devtools/inspector/_inspector.py:930, :1135-1141)
  • The existing TODO at :930 already says the fix belongs in ETRecord deserialization (cast the str keys back to ints in parse_etrecord, devtools/etrecord/_etrecord.py:795-806).
  • The key type is still inconsistent for other readers. For example, backends/qualcomm/tests/test_passes.py:649 indexes etrecord._debug_handle_map["forward"][0] with an int key, which only works on an unparsed object.
  • Normalizing in parse_etrecord, and switching _gen_resolve_debug_handles to int lookups, would fix every consumer and resolve the TODO. It would also remove the 3-line comment and nested helper, which CLAUDE.md's minimal-comments guidance argues against.
  • If the Inspector-side fix stays, update the stale instruction_id_to_num_outs_map: Dict[int, int] annotation at :935, since the lookup at :955 uses str keys.
• Lower confidence: the maps are now indexed eagerly (devtools/inspector/_inspector.py:1143-1151)
  • _debug_handle_map[FORWARD] and _instruction_id_to_num_outs_map[FORWARD] used to be indexed only inside the for event_block loop.
  • An ETRecord whose maps are None or lack a "forward" key, combined with no event blocks, now raises instead of continuing to gen_graphs_from_etrecord.
  • This is an edge case.

Testing

• The new test doesn't reach the delegate or num-outs paths (devtools/inspector/tests/inspector_test.py:393-430)
  • TestETRecord().get_test_model() builds a non-delegated model, and the synthetic Event has no delegate_debug_identifier.
  • So the delegate_map conversion, delegate_backend_name, and the crash above are never exercised. num_outputs, which comes from the third converted map, isn't asserted either.
  • Suggested additions:
    • A delegated case (e.g. XNNPACK) with an event whose delegate_debug_identifier is in the map.
    • A delegated event whose identifier is missing from the map.
    • assertEqual(event_from_object.num_outputs, event_from_file.num_outputs).

Recommendation

Request Changes

The nested delegate-map keys need normalizing, ideally in parse_etrecord as the existing TODO suggests. A delegated-model test case is needed to cover it. As written, the PR fixes non-delegated models but turns a silent failure into a TypeError for some delegated ones.

Note: the reviewers couldn't run Python in this environment, so the new test wasn't executed and all findings come from reading the code.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[u7k4rs6]

good catch, thanks. the nested delegate_map keys were still ints, so an unmapped delegate id would hit that TypeError. I switched it to round-trip the maps through json so they match what parse_etrecord gives, and added a test with a delegated event (one mapped, one not). kept it in the Inspector since other code reads the etrecord maps with int keys, but can move it to parse_etrecord if you'd rather.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[Gasoonjia]

thx it lgtm! i will stamp after ci passes!