safer check for isatty in fx/_utils.py #140876
Conversation
if no isatty method is defined, it's probably not a tty
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/140876
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ✅ No FailuresAs of commit f6ab9fd with merge base ce77409 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
torch/fx/_utils.py
Outdated
|
|
||
| if "colored" in kwargs and not sys.stdout.isatty(): | ||
| kwargs["colored"] = False | ||
| _stream_stdout = getattr(sys, 'stdout') |
There was a problem hiding this comment.
Is there a reason to do this? It seems equivalent to accessing sys.stdout
There was a problem hiding this comment.
Is there a reason to do this? It seems equivalent to accessing sys.stdout
I changed sys.stdout to an object that does not imitate _io.TextIOWrapper in my project, so there is no method isatty in this case and will raise AttributeError.
torch._dynamo.exc.InternalTorchDynamoError: AttributeError: 'F' object has no attribute 'isatty'
There was a problem hiding this comment.
Specifically, why say getattr(sys, 'stdout') when sys.stdout seems like it would work
There was a problem hiding this comment.
yeah lint complaining about it
There was a problem hiding this comment.
yeah lint complaining about it
My bad, sorry for the inconvenience. Unnecessary code has been removed in the latest commit in this PR.
|
@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 |
|
@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: 3 mandatory check(s) failed. The first few are: Dig deeper by viewing the failures on hud |
@ezyang I've made further changes trying to fix the issue for lint no-clang, could you please merge it again? Sorry for the inconvenience. :) |
|
Meh FIIINE |
|
@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 |
if no isatty method is defined, it's probably not a tty Pull Request resolved: pytorch#140876 Approved by: https://github.com/ezyang
5 participants
if no isatty method is defined, it's probably not a tty
cc @ezyang @SherlockNoMad @EikanWang @jgong5 @wenzhe-nrv