Add TORCHDYNAMO_EXTENDED_ADVICE (#137159) #137196
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/137196
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 e34366d with merge base 3192bde ( 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. |
| if not is_debug: | ||
| if ( | ||
| not is_debug | ||
| and os.getenv("TORCH_DISABLE_EXTENDED_DEBUG_INFO") is not None |
There was a problem hiding this comment.
nit: can you name this consistently with the other ones? Also, to make it discoverable, I'd tell users about it in the maybe_more_info string below.
There was a problem hiding this comment.
@ezyang thank you for the review! I didn't find a clear pattern to the naming of environment variables, some start with TORCH, some with PYTORCH (some don't have torch in the name at all), some include the module name some not. Is there a pattern I'm missing?
Anyway, let me know what would be a consistent name and I'll happily adjust and add to the maybe_more_info string.
There was a problem hiding this comment.
The envvar naming is indeed all over the place. But that is no excuse not to at least make them locally consistent. Specifically, the debug messages here refer to TORCHDYNAMO_EXTENDED_DEBUG_GUARD_ADDED and similar envvars, so something like TORCHDYNAMO_EXTENDED_ADVICE=0 feels consistent with what else we have
| if not is_debug: | ||
| if ( | ||
| not is_debug | ||
| and os.getenv("TORCHDYNAMO_EXTENDED_ADVICE=0") is not None |
There was a problem hiding this comment.
@ezyang ah, it should be
and os.getenv("TORCHDYNAMO_EXTENDED_ADVICE=0") is None
right?
Or do you mean, that the naming is still incorrect?
There was a problem hiding this comment.
don't pass =0 to the getenv. Grep for use of getenv in codebase, it will show the right pattern
There was a problem hiding this comment.
@ezyang
Got you. The current behavior is to show the extended info, so I would take that as default and use the following condition:
if not is_debug and os.getenv("TORCHDYNAMO_EXTENDED_ADVICE", "1") != "0"
To me it seems correct that there is extended info if TORCHDYNAMO_EXTENDED_ADVICE=1 and no extended info if TORCHDYNAMO_EXTENDED_ADVICE=0.
The user can then suppress the extended info by setting TORCHDYNAMO_EXTENDED_ADVICE=0. This will be noted in the maybe_more_info below.
Let me know if you agree, if so, I will make and commit the changes.
There was a problem hiding this comment.
Yes it defaults to 1. You may want to also handle empty string too.
There was a problem hiding this comment.
@ezyang Thanks, I made the changes and committed them. I know that this is a PR for a small issue and I appreciate the support and time taken to review.
| f'TORCHDYNAMO_EXTENDED_DEBUG_CREATE_SYMBOL="{sympy_expr}"' | ||
| f'TORCHDYNAMO_EXTENDED_DEBUG_CREATE_SYMBOL="{sympy_expr}" ' | ||
| "or to suppress this message run with " | ||
| 'TORCHDYNAMO_EXTENDED_ADVICE="0"' |
There was a problem hiding this comment.
nit you don't need the quotes
|
@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 |
Fixes pytorch#137159 Happy to contribute to this project for the first time. If I missed any contribution guidelines, please let me know, I'm happy to adjust. Pull Request resolved: pytorch#137196 Approved by: https://github.com/ezyang
Fixes #137159
Happy to contribute to this project for the first time. If I missed any contribution guidelines, please let me know, I'm happy to adjust.