From 934fa262507df05a51fc38b2b0ee3a07e9ab1078 Mon Sep 17 00:00:00 2001 From: Simon Brugman Date: Fri, 13 Sep 2024 17:51:13 +0200 Subject: [PATCH 01/15] Remove masOS stderr suppression `contextlib.redirect_stderr` works on Mac, no need to additional clause --- torchfix/__main__.py | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/torchfix/__main__.py b/torchfix/__main__.py index eb17658..80f11aa 100644 --- a/torchfix/__main__.py +++ b/torchfix/__main__.py @@ -2,7 +2,6 @@ import libcst.codemod as codemod import contextlib -import ctypes import sys import io @@ -22,22 +21,8 @@ def StderrSilencer(redirect: bool = True): if not redirect: yield - elif sys.platform != "darwin": - with contextlib.redirect_stderr(io.StringIO()): - yield - else: - # redirect_stderr does not work for some reason - # Workaround it by using good old dup2 to redirect - # stderr to /dev/null - libc = ctypes.CDLL("libc.dylib") - orig_stderr = libc.dup(2) - with open("/dev/null", "w") as f: - libc.dup2(f.fileno(), 2) - try: - yield - finally: - libc.dup2(orig_stderr, 2) - libc.close(orig_stderr) + with contextlib.redirect_stderr(io.StringIO()): + yield def _parse_args() -> argparse.Namespace: From 6277569036a310acba8a3f065a35faf5963bada9 Mon Sep 17 00:00:00 2001 From: Simon Brugman Date: Fri, 20 Sep 2024 21:20:23 +0200 Subject: [PATCH 02/15] Add a test for the stderr supression --- tests/test_torchfix.py | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/tests/test_torchfix.py b/tests/test_torchfix.py index 56fd05c..b899550 100644 --- a/tests/test_torchfix.py +++ b/tests/test_torchfix.py @@ -92,3 +92,32 @@ def test_errorcodes_distinct(): def test_parse_error_code_str(case, expected): assert process_error_code_str(case) == expected + + +def test_stderr_suppression(tmp_path): + data = "import torchvision.datasets as datasets\n" + data_path = tmp_path / "fixable.py" + data_path.write_text(data) + result = subprocess.run( + ["torchfix", "--select", "TOR203", "--fix", str(data_path)], + stderr=subprocess.PIPE, + text=True, + ) + assert ( + result.stderr + == "Finished checking 1 files.\nTransformed 1 files successfully.\n" + ) + + data = "import torchvision.datasets as datasets\n" + data_path = tmp_path / "fixable.py" + data_path.write_text(data) + result = subprocess.run( + ["torchfix", "--select", "TOR203", "--show-stderr", "--fix", str(data_path)], + stderr=subprocess.PIPE, + text=True, + ) + assert ( + result.stderr + == "Executing codemod...\nFailed to determine module name for {path}: '{path}' is not in the subpath of '' OR one path is relative and the other is absolute.\nFinished checking 1 files.\nTransformed 1 files successfully.\n" + ) + From e841f310c7ac0047c52f391be81f880745e95a9d Mon Sep 17 00:00:00 2001 From: Simon Brugman Date: Fri, 20 Sep 2024 21:21:45 +0200 Subject: [PATCH 03/15] CI: ensure `torchfix` cli works before tests --- .github/workflows/test-torchfix.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/test-torchfix.yml b/.github/workflows/test-torchfix.yml index 9a61e01..c288ce5 100644 --- a/.github/workflows/test-torchfix.yml +++ b/.github/workflows/test-torchfix.yml @@ -23,6 +23,9 @@ jobs: - name: Install TorchFix run: | pip3 install ".[dev]" + - name: Run torchfix CLI + run: | + torchfix --help - name: Run pytest run: | pytest tests From b83888d05d7d39243545f4a53ec13d63904ed46b Mon Sep 17 00:00:00 2001 From: Simon Brugman Date: Fri, 20 Sep 2024 21:23:38 +0200 Subject: [PATCH 04/15] Fix test --- tests/test_torchfix.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_torchfix.py b/tests/test_torchfix.py index b899550..75bba3b 100644 --- a/tests/test_torchfix.py +++ b/tests/test_torchfix.py @@ -1,3 +1,4 @@ +import subprocess from pathlib import Path from torchfix.torchfix import ( TorchChecker, @@ -118,6 +119,6 @@ def test_stderr_suppression(tmp_path): ) assert ( result.stderr - == "Executing codemod...\nFailed to determine module name for {path}: '{path}' is not in the subpath of '' OR one path is relative and the other is absolute.\nFinished checking 1 files.\nTransformed 1 files successfully.\n" + == f"Executing codemod...\nFailed to determine module name for {data_path}: '{data_path}' is not in the subpath of '' OR one path is relative and the other is absolute.\nFinished checking 1 files.\nTransformed 1 files successfully.\n" ) From cf63dd2723d83f9556396e7556b760db88b96c85 Mon Sep 17 00:00:00 2001 From: Simon Brugman Date: Fri, 20 Sep 2024 21:29:50 +0200 Subject: [PATCH 05/15] CI: increase pytest verbosity --- .github/workflows/test-torchfix.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test-torchfix.yml b/.github/workflows/test-torchfix.yml index c288ce5..4cd8d7f 100644 --- a/.github/workflows/test-torchfix.yml +++ b/.github/workflows/test-torchfix.yml @@ -28,7 +28,7 @@ jobs: torchfix --help - name: Run pytest run: | - pytest tests + pytest -vv tests - name: Run flake8 run: | flake8 From 4f2d18f105d777dc0ae4c40a5801ff2ac4308a26 Mon Sep 17 00:00:00 2001 From: Simon Brugman Date: Fri, 20 Sep 2024 21:40:19 +0200 Subject: [PATCH 06/15] TST: add explicit check keyword to `subprocess.run` --- tests/test_torchfix.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_torchfix.py b/tests/test_torchfix.py index 75bba3b..16cc25a 100644 --- a/tests/test_torchfix.py +++ b/tests/test_torchfix.py @@ -103,6 +103,7 @@ def test_stderr_suppression(tmp_path): ["torchfix", "--select", "TOR203", "--fix", str(data_path)], stderr=subprocess.PIPE, text=True, + check=True, ) assert ( result.stderr @@ -116,6 +117,7 @@ def test_stderr_suppression(tmp_path): ["torchfix", "--select", "TOR203", "--show-stderr", "--fix", str(data_path)], stderr=subprocess.PIPE, text=True, + check=True, ) assert ( result.stderr From 123d1ee0e9669c828a14a22918af929c4d5b0cdf Mon Sep 17 00:00:00 2001 From: Simon Brugman Date: Fri, 20 Sep 2024 21:51:57 +0200 Subject: [PATCH 07/15] Fix stderr suppression --- torchfix/__main__.py | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/torchfix/__main__.py b/torchfix/__main__.py index 80f11aa..5400f1b 100644 --- a/torchfix/__main__.py +++ b/torchfix/__main__.py @@ -16,15 +16,6 @@ from .common import CYAN, ENDC -# Should get rid of this code eventually. -@contextlib.contextmanager -def StderrSilencer(redirect: bool = True): - if not redirect: - yield - with contextlib.redirect_stderr(io.StringIO()): - yield - - def _parse_args() -> argparse.Namespace: parser = argparse.ArgumentParser() @@ -87,7 +78,12 @@ def main() -> None: command_instance = TorchCodemod(codemod.CodemodContext(), config) DIFF_CONTEXT = 5 try: - with StderrSilencer(not args.show_stderr): + supress_stderr = ( + contextlib.redirect_stderr(io.StringIO()) + if not args.show_stderr + else contextlib.nullcontext() + ) + with supress_stderr: result = codemod.parallel_exec_transform_with_prettyprint( command_instance, torch_files, From 2fef0cd660e3ebe9dab556a48a3db42458f28e0d Mon Sep 17 00:00:00 2001 From: Simon Brugman Date: Fri, 20 Sep 2024 21:55:15 +0200 Subject: [PATCH 08/15] Flake8 --- tests/test_torchfix.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/test_torchfix.py b/tests/test_torchfix.py index 16cc25a..5213bc5 100644 --- a/tests/test_torchfix.py +++ b/tests/test_torchfix.py @@ -121,6 +121,7 @@ def test_stderr_suppression(tmp_path): ) assert ( result.stderr - == f"Executing codemod...\nFailed to determine module name for {data_path}: '{data_path}' is not in the subpath of '' OR one path is relative and the other is absolute.\nFinished checking 1 files.\nTransformed 1 files successfully.\n" + == f"Executing codemod...\nFailed to determine module name for {data_path}: '{data_path}' is not in the subpath" + " of '' OR one path is relative and the other is absolute.\nFinished checking 1 files.\nTransformed 1 files " + "successfully.\n" ) - From 6e408e57e8db2c6ddda48ae36461700d72977c3e Mon Sep 17 00:00:00 2001 From: Simon Brugman Date: Fri, 20 Sep 2024 21:58:20 +0200 Subject: [PATCH 09/15] Flake8 --- tests/test_torchfix.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/test_torchfix.py b/tests/test_torchfix.py index 5213bc5..8492d75 100644 --- a/tests/test_torchfix.py +++ b/tests/test_torchfix.py @@ -121,7 +121,8 @@ def test_stderr_suppression(tmp_path): ) assert ( result.stderr - == f"Executing codemod...\nFailed to determine module name for {data_path}: '{data_path}' is not in the subpath" - " of '' OR one path is relative and the other is absolute.\nFinished checking 1 files.\nTransformed 1 files " + == f"Executing codemod...\nFailed to determine module name for {data_path}: " + f"'{data_path}' is not in the subpath of '' OR one path is relative and the " + "other is absolute.\nFinished checking 1 files.\nTransformed 1 files " "successfully.\n" ) From 41e042261ace6ba2ac80a98338aceff6ee4c7342 Mon Sep 17 00:00:00 2001 From: Simon Brugman Date: Sat, 21 Sep 2024 00:16:23 +0200 Subject: [PATCH 10/15] CI: also test macOS x86 and windows --- .github/workflows/test-torchfix.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test-torchfix.yml b/.github/workflows/test-torchfix.yml index 4cd8d7f..e54a681 100644 --- a/.github/workflows/test-torchfix.yml +++ b/.github/workflows/test-torchfix.yml @@ -8,7 +8,7 @@ jobs: test-torchfix: strategy: matrix: - os: [ubuntu-latest, macos-latest] + os: [ubuntu-latest, macos-latest, macos-latest-large, windows-latest] runs-on: ${{ matrix.os }} steps: - name: Checkout From 3e40b713588e18c8595f47f710a130fadc2af83b Mon Sep 17 00:00:00 2001 From: Simon Brugman Date: Sat, 21 Sep 2024 00:21:44 +0200 Subject: [PATCH 11/15] TST: windows line separator --- tests/test_torchfix.py | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/tests/test_torchfix.py b/tests/test_torchfix.py index 8492d75..5a644a2 100644 --- a/tests/test_torchfix.py +++ b/tests/test_torchfix.py @@ -1,3 +1,4 @@ +import os import subprocess from pathlib import Path from torchfix.torchfix import ( @@ -96,33 +97,33 @@ def test_parse_error_code_str(case, expected): def test_stderr_suppression(tmp_path): - data = "import torchvision.datasets as datasets\n" + data = f"import torchvision.datasets as datasets{os.linesep}" data_path = tmp_path / "fixable.py" data_path.write_text(data) result = subprocess.run( ["torchfix", "--select", "TOR203", "--fix", str(data_path)], stderr=subprocess.PIPE, text=True, - check=True, + check=False, ) assert ( - result.stderr - == "Finished checking 1 files.\nTransformed 1 files successfully.\n" + result.stderr == f"Finished checking 1 files.{os.linesep}" + f"Transformed 1 files successfully.{os.linesep}" ) - data = "import torchvision.datasets as datasets\n" + data = f"import torchvision.datasets as datasets{os.linesep}" data_path = tmp_path / "fixable.py" data_path.write_text(data) result = subprocess.run( ["torchfix", "--select", "TOR203", "--show-stderr", "--fix", str(data_path)], stderr=subprocess.PIPE, text=True, - check=True, + check=False, ) assert ( - result.stderr - == f"Executing codemod...\nFailed to determine module name for {data_path}: " - f"'{data_path}' is not in the subpath of '' OR one path is relative and the " - "other is absolute.\nFinished checking 1 files.\nTransformed 1 files " - "successfully.\n" + result.stderr == f"Executing codemod...{os.linesep}" + f"Failed to determine module name for {data_path}: '{data_path}' is not in the " + f"subpath of '' OR one path is relative and the other is absolute.{os.linesep}" + f"Finished checking 1 files.{os.linesep}" + f"Transformed 1 files successfully.{os.linesep}" ) From faf6174fa2ecc0f9872fb7ca5e679a0c03d192fa Mon Sep 17 00:00:00 2001 From: Simon Brugman Date: Sat, 21 Sep 2024 00:30:42 +0200 Subject: [PATCH 12/15] CI: Conditional printing of architecture --- .github/workflows/test-torchfix.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/test-torchfix.yml b/.github/workflows/test-torchfix.yml index e54a681..bb007d1 100644 --- a/.github/workflows/test-torchfix.yml +++ b/.github/workflows/test-torchfix.yml @@ -13,6 +13,10 @@ jobs: steps: - name: Checkout uses: actions/checkout@v4 + - name: Show CPU architecture + if: matrix.os == 'macos-latest-large' || matrix.os == 'macos-latest' + run: | + uname -m - uses: actions/setup-python@v5 with: python-version: '3.10' From 68508459f8ec5a9e390d5b2757d6b7a6cd3719b1 Mon Sep 17 00:00:00 2001 From: Simon Brugman Date: Sat, 21 Sep 2024 00:33:11 +0200 Subject: [PATCH 13/15] TST: revert linesep --- tests/test_torchfix.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_torchfix.py b/tests/test_torchfix.py index 5a644a2..f48d11d 100644 --- a/tests/test_torchfix.py +++ b/tests/test_torchfix.py @@ -107,8 +107,8 @@ def test_stderr_suppression(tmp_path): check=False, ) assert ( - result.stderr == f"Finished checking 1 files.{os.linesep}" - f"Transformed 1 files successfully.{os.linesep}" + result.stderr == "Finished checking 1 files.\n" + "Transformed 1 files successfully.\n" ) data = f"import torchvision.datasets as datasets{os.linesep}" From 154613dcb42e3c1f1747cfd542ac3d3d2c7723cd Mon Sep 17 00:00:00 2001 From: Simon Brugman Date: Sat, 21 Sep 2024 00:37:00 +0200 Subject: [PATCH 14/15] TST: fixing _that other_ windows issue... --- tests/test_torchfix.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/test_torchfix.py b/tests/test_torchfix.py index f48d11d..5abdb49 100644 --- a/tests/test_torchfix.py +++ b/tests/test_torchfix.py @@ -120,10 +120,11 @@ def test_stderr_suppression(tmp_path): text=True, check=False, ) + data_path = str(data_path).replace("\\", "\\\\") assert ( - result.stderr == f"Executing codemod...{os.linesep}" + result.stderr == f"Executing codemod...\n" f"Failed to determine module name for {data_path}: '{data_path}' is not in the " - f"subpath of '' OR one path is relative and the other is absolute.{os.linesep}" - f"Finished checking 1 files.{os.linesep}" - f"Transformed 1 files successfully.{os.linesep}" + f"subpath of '' OR one path is relative and the other is absolute.\n" + f"Finished checking 1 files.\n" + f"Transformed 1 files successfully.\n" ) From 77f30561016717e14bf707505075bc4d0124ec1d Mon Sep 17 00:00:00 2001 From: Simon Brugman Date: Sat, 21 Sep 2024 00:42:28 +0200 Subject: [PATCH 15/15] TST: alter expected Windows paths --- tests/test_torchfix.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_torchfix.py b/tests/test_torchfix.py index 5abdb49..01ee556 100644 --- a/tests/test_torchfix.py +++ b/tests/test_torchfix.py @@ -120,9 +120,9 @@ def test_stderr_suppression(tmp_path): text=True, check=False, ) - data_path = str(data_path).replace("\\", "\\\\") + expected = result.stderr.replace("\\\\", "\\") assert ( - result.stderr == f"Executing codemod...\n" + expected == f"Executing codemod...\n" f"Failed to determine module name for {data_path}: '{data_path}' is not in the " f"subpath of '' OR one path is relative and the other is absolute.\n" f"Finished checking 1 files.\n"