Skip to content
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

Remove macOS stderr suppression #76

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

sbrugman
Copy link
Collaborator

@sbrugman sbrugman commented Sep 13, 2024

contextlib.redirect_stderr works on Mac, no need to additional clause

`contextlib.redirect_stderr` works on Mac, no need to additional clause
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 13, 2024
@sbrugman sbrugman marked this pull request as draft September 13, 2024 15:52
@sbrugman sbrugman marked this pull request as ready for review September 20, 2024 19:59
@sbrugman
Copy link
Collaborator Author

We actually had a subtle bug in the previous implementation: when --show-stderr was provided, the program would exit because None is not a valid contextmanager. The contextlib.nullcontext is an elegant alternative for the time being.

Changes:

  • FIX: avoid crash when --show-stderr is passed
  • ENH: remove obviated dup2 special suppression for macOS
  • CI: Increase verbosity of pytest for more informative errors.
  • CI: Test the torchfix binary is available (used by the test)
  • TST: Add a test for --show-stderr

@kit1980
Copy link
Contributor

kit1980 commented Sep 20, 2024

Thanks!
I'll double check on an x86 Mac where the issue was happening.

And I need to follow-up on Instagram/LibCST#944 eventually to fix the root cause...

@sbrugman
Copy link
Collaborator Author

@kit1980 Github Actions also offers macOS x86 runners - I've added that one and the test pass. Windows is also included now.

@kit1980
Copy link
Contributor

kit1980 commented Sep 24, 2024

I tried this on my x86 macbook, and the suppression doesn't work.
tests/test_torchfix.py::test_stderr_suppression actually passed, but running torchfix . causes tons of noise:

torchfix .
Failed to determine module name for /Users/sdym/repos/pytorch-release/vision/examples/cpp/script_model.py: '/Users/sdym/repos/pytorch-release/vision/examples/cpp/script_model.py' is not in the subpath of '' OR one path is relative and the other is absolute.
Failed to determine module name for /Users/sdym/repos/pytorch-release/vision/.github/process_commit.py: '/Users/sdym/repos/pytorch-release/vision/.github/process_commit.py' is not in the subpath of '' OR one path is relative and the other is absolute.

@kit1980 kit1980 requested review from malfet and kit1980 September 24, 2024 20:51
@sbrugman
Copy link
Collaborator Author

Pity, that version of Mac then probably outputs the stderr to stdout and leaves stderr empty...

text=True,
check=False,
)
assert (
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should also check the stdout here for the macOS issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants