Refactor FxGraphDrawer to use HTML-like labels #137726
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/137726
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (1 Unrelated Failure)As of commit ed565e0 with merge base 889717a ( FLAKY - The following job failed but was likely due to flakiness present on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
this is fast! took a look. no major concerns except address linters and wait for CIs |
| # to specify the shape attribute for the dot graph. For example, passing | ||
| # INDUCTOR_DOT_GRAPH_SHAPE_SVG = "none" would let us generate HTML-like lables | ||
| # to workaround the above failure. | ||
| dot_graph_shape = os.environ.get("INDUCTOR_DOT_GRAPH_SHAPE_SVG", None) |
There was a problem hiding this comment.
Is there no longer a way to generate this ?
There was a problem hiding this comment.
My reasoning for removing the config flag is that it was originally added to resolve a bug that is now fixed by this PR.
However, other shapes do work with html-like labels, so leaving it in wouldn't hurt anything. It's definitely a low impact config flag so if we're trying to simplify the config file, it would be a good one to remove.
There was a problem hiding this comment.
ah, didnt realize that this config was just for the bug.
There was a problem hiding this comment.
Have we ever documented it? If so, it probably should be kept around
There was a problem hiding this comment.
I did a google search and nothing came up except the source code itself, so I don't think it's been documented.
8540e76
to
f36e734
Compare
I will add a test |
f36e734
to
5450df2
Compare
3c7c5a9
to
95cda94
Compare
8cf7ac4
to
db00a36
Compare
083c170
to
c437102
Compare
7422e92
to
d6f636e
Compare
962184d
to
e9f7769
Compare
4a64d21
to
35487e6
Compare
| # to specify the shape attribute for the dot graph. For example, passing | ||
| # INDUCTOR_DOT_GRAPH_SHAPE_SVG = "none" would let us generate HTML-like lables | ||
| # to workaround the above failure. | ||
| dot_graph_shape = os.environ.get("INDUCTOR_DOT_GRAPH_SHAPE_SVG", None) |
There was a problem hiding this comment.
Have we ever documented it? If so, it probably should be kept around
94b7ba7
to
fa78566
Compare
|
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Merge failedReason: 2 jobs have failed, first few of them are: inductor / linux-jammy-cpu-py3.12-gcc11-inductor-halide / build, trunk / linux-focal-rocm6.2-py3.10 / build Details for Dev Infra teamRaised by workflow job |
fixes #137499 Testing Plan: New unit test.
fa78566
to
ed565e0
Compare
|
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
|
The merge job was canceled or timed out. This most often happen if two merge requests were issued for the same PR, or if merge job was waiting for more than 6 hours for tests to finish. In later case, please do not hesitate to reissue the merge command |
Fixes #137499
Testing: Added a new unit test to make sure that the regression case succeeds.
I'm debating about whether to make the borders visible. I'm partial to no borders, but it might make it harder for some people to read?
Vs.
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @ipiszy @yf225 @chenyang78 @kadeng @muchulee8 @ColinPeppler @amjames @desertfire @chauhang @aakhundov