-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add flag _metrics_log_runtime
to disable runtime metric logging by default
#153506
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/153506
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit e3f7169 with merge base 72a3c8d ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
flag _metrics_log_runtime
to disable runtime metric logging by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to fix tests..
torch/_inductor/compile_fx.py
Outdated
metrics.num_bytes_accessed += num_bytes | ||
metrics.node_runtimes += node_runtimes | ||
metrics.nodes_num_elem += nodes_num_elem | ||
if config._metrics_log_runtime: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just move this to TORCH_LOGS="inductor_metrics" ? it's a small change to add the logs - see https://github.com/pytorch/pytorch/pull/147248/files
2739678
to
7a15fa1
Compare
de5d03a
to
f5b3f92
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
"inductor backend is not available", | ||
) | ||
def test_save_and_load_inductor(self): | ||
torch._logging.set_logs(inductor_metrics=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set back to none at end
@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: 1 mandatory check(s) failed. The first few are: Dig deeper by viewing the failures on hud |
@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: 1 mandatory check(s) failed. The first few are: Dig deeper by viewing the failures on hud |
cfc623b
to
e3f7169
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 |
#152708 expanded support of
get_estimated_runtime
to many more types ofSchedulerNodes
. This caused an increase in compile time because we're always callingget_estimated_runtime
to populate the metrics table. This PR adds a flag for this logging, which reduces the instruction count by 8%. Long term, we should probably merge metrics.py with TORCH_LOGS/tlparse (suggestion from @xmfan).Update: added support for TORCH_LOGS for the metrics logging.
Test Plan:
mm_loop.py and many existing tests cover.
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @ipiszy @chenyang78 @kadeng @muchulee8 @amjames @chauhang @aakhundov