Repository navigation
Conversation
|
@FindHao has exported this pull request. If you are a Meta employee, you can view the originating Diff in D121418449. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved critical session-partitioning and moderate fixture/assertion consistency issues remain.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (3)
What changed in this PR
Regenerates autotune example traces and updates parsing to split call-site sessions by autotune key.
Changes:
- Captures per-launch occurrences and autotune callbacks.
- Produces per-key analysis sessions.
- Updates launch-count expectations and adds regression coverage.
| File | Summary |
|---|---|
tritonparse/parse/trace_processor.py |
Captures launch and autotune callback metadata. |
tritonparse/parse/event_diff.py |
Splits sessions by key; needs an autotune_key fallback when launch signatures are unavailable. |
tests/cpu/test_kernel_query.py |
Assertion records 1041 launches while the regenerated fixture is stated as 1055. |
tests/cpu/test_autotune_session_split.py |
Adds session-splitting tests; comment contains “sames scalars” typo. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| config_args = _find_config_args(group_infos) | ||
| signature_by_group = { | ||
| group_hash: _launch_key_signature(extracted, config_args) | ||
| for group_hash, _comp_hash, extracted in group_infos |
| kernel_dict = {k.name: k for k in result} | ||
| self.assertEqual(kernel_dict["fused_op_kernel"].total_launches, 4) | ||
| self.assertEqual(kernel_dict["matmul_kernel"].total_launches, 1078) | ||
| self.assertEqual(kernel_dict["matmul_kernel"].total_launches, 1041) |
| self.assertEqual(set(by_winner), {"hash_a", "hash_b"}) | ||
|
|
||
| # Each key round keeps single-valued args: M/N are constant | ||
| # within the round (sames scalars), only the true config param |
999478d to
0a2bb0a
Compare
Summary: Pull Request resolved: #440 Regenerate the `triton` and `inductor` example traces with `tritonparse.tools.generate_examples` (triton 3.8.0, torch 2.15 dev+cu130, H100) on top of the parent diff's autotune session split, so the shipped website examples and `tests/example_output` fixtures reflect per-key sub-sessions. What changed in the new traces: - The triton example's call-site matmul session now splits into two per-key sub-sessions (M=N=16 vs M=32,N=32): 1 -> 2 `autotune_analysis` events with `<site>:<key>` session ids. - matmul launch count 1078 -> 1041 (autotune benchmark jitter; refreshed to match the review-fixed parent, whose sub-session ids now carry full 16-char key signatures). Test-only update alongside the regen: - `test_list_kernels_multiple`: matmul launches 1078 -> 1041 (the comment already says to refresh on regen). Differential Revision: D121418449
0a2bb0a to
21a37cc
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Autotune sessions and callback results can be partitioned or matched incorrectly.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 2
Open (4)
Resolved since last review (2)
| for arg_name, per_hash in values_by_arg.items(): | ||
| if len(per_hash) <= 1: | ||
| continue | ||
| if not all(len(values) == 1 for values in per_hash.values()): | ||
| continue | ||
| # The values must actually disagree across hashes; a constant arg | ||
| # is not a config param even though it trivially has "one value | ||
| # per hash". | ||
| if len(set().union(*per_hash.values())) > 1: | ||
| config_args.add(arg_name) |
Summary: Pull Request resolved: #440 Regenerate the `triton` and `inductor` example traces with `tritonparse.tools.generate_examples` (triton 3.8.0, torch 2.15 dev+cu130, H100) on top of the parent diff's autotune session split, so the shipped website examples and `tests/example_output` fixtures reflect per-key sub-sessions. What changed in the new traces: - The triton example's call-site matmul session now splits into two per-key sub-sessions (M=N=16 vs M=32,N=32): 1 -> 2 `autotune_analysis` events with `<site>:<key>` session ids. - matmul launch count 1078 -> 1041 (autotune benchmark jitter; refreshed to match the review-fixed parent, whose sub-session ids now carry full 16-char key signatures). Test-only update alongside the regen: - `test_list_kernels_multiple`: matmul launches 1078 -> 1041 (the comment already says to refresh on regen). Differential Revision: D121418449
21a37cc to
4b7cd50
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical launch-loss and callback-attribution defects must be resolved before approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 4
Open (7)
Note-only launches are omitted from emitted analysis · New Partial config matches misattribute callback results · New Preserve key arguments when deriving config fields Use autotune key when launch signature is unavailable Nested descriptors lose key-dependent fields · New Synchronize fixture launch count with assertion Fix grammatical typo in test comment
| occurrences = [ | ||
| record | ||
| for record in occurrences | ||
| if record.get("launch_group_hash") not in skipped_note_groups | ||
| ] |
| for name, expected in pairs.items(): | ||
| if name == "num_warps" and winner_warps_base is not None: | ||
| actual = winner_warps_base | ||
| elif name in extracted and not _is_tensor_like_value(extracted[name]): | ||
| actual = _extract_scalar_value(extracted[name]) | ||
| elif name in compilation_metadata: | ||
| actual = compilation_metadata[name] | ||
| else: | ||
| continue | ||
| compared += 1 | ||
| if _config_value_matches(expected, actual): | ||
| matched += 1 | ||
| if compared > 0 and matched == compared: | ||
| return index |
| elif isinstance(arg_val, dict): | ||
| # Descriptors and foreign wrappers: keep identity metadata | ||
| # but drop volatile nested data_ptrs for stability. | ||
| payload[arg_name] = _drop_data_ptrs(arg_val) |
Summary: Autotune session ids hash only the user call stack, so repeated Autotuner.run() invocations at the same call site with different keys (e.g. a loop over M=N=16 then M=N=32, as in the regenerated example trace) collapse into one session. Per-config args then become multi-valued distributions that the viewer renders as an ellipsis. Expand each call-site session into per-key sub-sessions at analysis time: launches are grouped by a signature over runtime args (config params excluded, tensor data_ptrs excluded), compilations attach by referenced hash, and each sub-session resolves its own winner and AutotuneListener result (matched via best_config). Unsplit sessions keep their plain session id and behave exactly as before. Ingest records per-launch occurrence mapping and keeps autotune results as a list (previously last-wins); the coarse last-wins winner map is replaced by per-sub-session resolution from occurrence records. Differential Revision: D121412109
Summary: Pull Request resolved: #440 Regenerate the `triton` and `inductor` example traces with `tritonparse.tools.generate_examples` (triton 3.8.0, torch 2.15 dev+cu130, H100) on top of the parent diff's autotune session split, so the shipped website examples and `tests/example_output` fixtures reflect per-key sub-sessions. What changed in the new traces: - The triton example's call-site matmul session now splits into two per-key sub-sessions (M=N=16 vs M=32,N=32): 1 -> 2 `autotune_analysis` events with `<site>:<key>` session ids. - matmul launch count 1078 -> 1041 (autotune benchmark jitter; refreshed to match the review-fixed parent, whose sub-session ids now carry full 16-char key signatures). Test-only update alongside the regen: - `test_list_kernels_multiple`: matmul launches 1078 -> 1041 (the comment already says to refresh on regen). Differential Revision: D121418449
4b7cd50 to
57d8dba
Compare
|
This pull request has been merged in 5edb065. |



Summary:
Regenerate the
tritonandinductorexample traces withtritonparse.tools.generate_examples(triton 3.8.0, torch 2.15 dev+cu130, H100) on top of the parent diff's autotune session split, so the shipped website examples andtests/example_outputfixtures reflect per-key sub-sessions.What changed in the new traces:
autotune_analysisevents with<site>:<key>session ids.Test-only update alongside the regen:
test_list_kernels_multiple: matmul launches 1078 -> 1041 (the comment already says to refresh on regen).Differential Revision: D121418449