Add compiled_autograd_id to CompileId #83

xmfan · 2024-12-06T05:58:33Z

land with pytorch/pytorch#141907

xmfan · 2024-12-06T06:00:13Z

tests/integration_test.rs

    // Read the test file
-    // simple.log was generated from the following:
+    // artifacts.log was generated from the following:
+    // NOTE: test output looks nothing like artifacts.log


@jamesjwu do you remember which test we need to run for this?

tests/integration_test.rs

ezyang · 2024-12-06T14:36:28Z

Plz fix lint

ezyang · 2024-12-06T14:37:09Z

tests/integration_test.rs

-        "0_0_0/aot_forward_graph",
-        "0_0_0/dynamo_output_graph",
+        "-_0_0_0/aot_forward_graph",
+        "-_0_0_0/dynamo_output_graph",


This seems not so nice. Just omit the -_ delimiter when there is no compiled autograd. There isn't ambiguity because the first 0 can only be a digit anyway.

ezyang · 2024-12-06T16:16:43Z

src/types.rs

+        dynamo_id: Option<DynamoId>,
+    },
+    // When user calls torch.compile
+    UserInitiated(DynamoId)


Why do you want to have an untagged union here? Why not just have a single struct with optional fields everywhere?

(I am generally a big union stan but JSON is a lot more straightforward to deserialize and evolve if you don't do unions, since it has no built-in notion of union)

Untagged to be backwards compatible with old logs, and to keep evolving with a flat json structure. The tricky part going forward is to maintain the ordering of fields and the ordering of enums. If that's too much, we can revert back to just a struct of optionals since the data format doesn't change

The single struct with optionals feels too permissive because it's not obvious from looking at the definition what set of CompileId you can expect.

ezyang · 2024-12-09T18:09:08Z

tests/integration_test.rs

-    // simple.log was generated from the following:
-    // TORCH_TRACE=~/trace_logs/test python test/inductor/test_torchinductor.py  -k TORCH_TRACE=~/trace_logs/comp_metrics python test/dynamo/test_misc.py -k test_graph_break_compilation_metrics
+    // comp_metrics.log was generated from the following:
+    // TORCH_TRACE=~/trace_logs/comp_metrics python test/dynamo/test_misc.py -k test_graph_break_compilation_metrics


FWIW, you don't have to regenerate the old logs, because it's important for us to be compatible with parsing old logs, so having old logs around is useful too.

(I wouldn't change your code here, because this becomes successively less important as time goes on lol)

tlparse PR: ezyang/tlparse#83 cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames yf225 [ghstack-poisoned]

ezyang · 2024-12-10T03:04:19Z

src/lib.rs

+                        let attempt_str = attempt.map_or("-".to_string(), |v| v.to_string());
+
+                        if let Some(ca_id) = compiled_autograd_id {
+                            format!("{ca_id}_{frame_id_str}_{frame_compile_id_str}_{attempt_str}")


Since the file naming here isn't load bearing, I wonder if we should do something wordier like "compiled_autograd_{ca_id}_..." so it's obvious that compiled autograd is involved here. Well, I guess the filename doesn't matter that much.

we could use abbreviations like ca{id}_... to keep things compact and to stick under the 255 char limit for windows

ezyang · 2024-12-10T03:05:56Z

src/parsers.rs

+                } else {
+                    format!("{frame_id_str}_{frame_compile_id_str}_{attempt_str}")
+                }
+            },


I think this and your earlier site have to be updated in lockstep, so now that it is this complicated you should actually factor this out to a dedicated function.

ezyang · 2024-12-10T03:35:14Z

src/types.rs

+        if let Some(attempt) = self.attempt {
+            if attempt != 0 {
+                write!(f, "_{}", attempt)?;
+            }


OK, so this is for the display, and I think it's worth bikeshedding this part a little. My main concern is that if we render as 0/1/0_0, people are not going to know what this means. In the old days, I designed the compile id to be compact so that I could put it in front of every log message so you knew what compile id it was associated with. But now in the tlparse era it is less important for it to be compact (though I guess it is still nice for TORCH_TRACE to be compact). I feel like there are two ways we can address this. One is to have some expanded syntax for compiled autograd like "compiled autograd 0: 1/0_0" which makes it obvious it's compiled autograd. The other is to make this an HTML renderer and then add a tooltip so when you mouse over you see the complete thing. I'm happy to defer to implementor's privilege here but at the very minimum, if you are going to stick to this syntax, you need to update the help text in tlparse to explain what it does.

By the way, when all three latter fields are empty, I think it's still helpful to just elide the rhs entirely, so you just have "compiled autograd 0". You can't do this without a special compiled autograd sigil though, because o/w it's ambiguous!

I think we should render the id as hierarchical in the tlparse, and the different id fields would be displayed separately:

That's ok too! But it is nice to have a globally unique id too

if we want a globally unique id that's visible to the user, and that's consistent with how we save it in our datastores, i think we should just keep using [x/y/z_a] and [y/z_a] out of simplicity.

So I have another question for you about future proofing. Another place we may need to insert another level of hierarchy is for DDPOptimizer, which splits a single Dynamo graph into multiple subgraphs. Here, this would be done most logically by adding another level of hierarchy below z, i.e. [y/z_a/h]. When a is 0 (and therefore elided), how do we disambiguate between [x/y/z] and [y/z/h]?

Ok regathering the requirements:

We want CompileId to be a program-level uid

We want this displayed to the user so it should be short. It's not necessary for tlparse, but it's very nice to have for TORCH_TRACE and internal dashboards

We want this used in downstream data pipelines so it should be easily parsable from a string

One option is only allow eliding arguments that use prefix/suffix unique tokens: [!0/1/0] vs [0/1/@0]. The compact form is favorable to be directly used by internal dashboards widegets where it might be hard to have custom rendering

We probably want compile directory to also be unique, but special tokens don't work well with filesystems, so we could continue with a no eliding approach: 0_0_0_0_0

For the filesystem directory I really don't care haha, just as long as it's unique

ezyang

Nothing here is irreversible so go for it. I would appreciate if you considered the comments though!

tlparse PR: ezyang/tlparse#83 cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames yf225 [ghstack-poisoned]

tlparse PR: ezyang/tlparse#83 cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx ipiszy yf225 chenyang78 kadeng muchulee8 ColinPeppler amjames desertfire chauhang aakhundov [ghstack-poisoned]

tlparse PR: ezyang/tlparse#83 Pull Request resolved: #141907 Approved by: https://github.com/ezyang

Summary: tlparse PR: ezyang/tlparse#83 X-link: pytorch/pytorch#141907 Approved by: https://github.com/ezyang Reviewed By: huydhn Differential Revision: D67551013 Pulled By: xmfan fbshipit-source-id: 86af69d24568874ce4eb72faafa7084a293e957a

xmfan commented Dec 6, 2024

View reviewed changes

xmfan force-pushed the ca branch 2 times, most recently from 8ff043f to fb20d4d Compare December 6, 2024 06:26

ezyang reviewed Dec 6, 2024

View reviewed changes

xmfan marked this pull request as ready for review December 6, 2024 23:29

xmfan mentioned this pull request Dec 6, 2024

[ca] add compiled autograd to CompileId pytorch/pytorch#141907

Closed

ezyang reviewed Dec 9, 2024

View reviewed changes

xmfan force-pushed the ca branch from f7b618d to b3ac3c4 Compare December 9, 2024 20:41

xmfan changed the title ~~Introduce CompileId variants and CompiledAutogradInitiated CompileId variant~~ Add compiled_autograd_id to CompileId Dec 9, 2024

Mark everything in CompileId as optional, add compiled_autograd_id

a6a444d

xmfan force-pushed the ca branch from b3ac3c4 to a6a444d Compare December 9, 2024 20:43

ezyang reviewed Dec 10, 2024

View reviewed changes

ezyang approved these changes Dec 10, 2024

View reviewed changes

xmfan added 4 commits December 10, 2024 19:46

address comments

92a60c1

add prefix to compiled_autograd_id

300ffd3

fix tests

0d7bae3

update html readme

baa6da4

pytorchmergebot pushed a commit to pytorch/pytorch that referenced this pull request Dec 21, 2024

[ca] add compiled autograd to CompileId (#141907)

4ee166b

tlparse PR: ezyang/tlparse#83 Pull Request resolved: #141907 Approved by: https://github.com/ezyang

xmfan merged commit 62ab847 into ezyang:main Dec 21, 2024
14 checks passed

xmfan mentioned this pull request Jan 6, 2025

No compile id for compiled autograd artifacts in tlparse pytorch/pytorch#133127

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add compiled_autograd_id to CompileId #83

Add compiled_autograd_id to CompileId #83

xmfan commented Dec 6, 2024

xmfan Dec 6, 2024

ezyang commented Dec 6, 2024

ezyang Dec 6, 2024

ezyang Dec 6, 2024

ezyang Dec 6, 2024

xmfan Dec 6, 2024 •

edited

Loading

ezyang Dec 9, 2024

ezyang Dec 9, 2024

ezyang Dec 10, 2024

xmfan Dec 10, 2024

ezyang Dec 10, 2024

ezyang Dec 10, 2024

xmfan Dec 10, 2024

ezyang Dec 10, 2024

xmfan Dec 11, 2024

ezyang Dec 11, 2024

xmfan Dec 11, 2024

ezyang Dec 11, 2024

ezyang left a comment

Add compiled_autograd_id to CompileId #83

Add compiled_autograd_id to CompileId #83

Conversation

xmfan commented Dec 6, 2024

Choose a reason for hiding this comment

ezyang commented Dec 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xmfan Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezyang left a comment

Choose a reason for hiding this comment

xmfan Dec 6, 2024 •

edited

Loading