From 61fd4f435b214c9666857101dd2634ed2ecb964f Mon Sep 17 00:00:00 2001 From: Fabrice Normandin Date: Fri, 11 Oct 2024 11:46:43 -0400 Subject: [PATCH 1/7] Add test to reproduce issue Signed-off-by: Fabrice Normandin --- tests/cli/test_code.py | 75 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 tests/cli/test_code.py diff --git a/tests/cli/test_code.py b/tests/cli/test_code.py new file mode 100644 index 00000000..bbc236b2 --- /dev/null +++ b/tests/cli/test_code.py @@ -0,0 +1,75 @@ +"""Unit tests for the `milatools.cli.code` module. + +TODO: There are quite a few tests in `tests/integration/test_code.py` that could be +moved here, since some of them aren't exactly "integration" tests. +""" + +import sys +from unittest.mock import AsyncMock, Mock + +import pytest + +import milatools.cli.code +import milatools.cli.utils +from milatools.cli.utils import running_inside_WSL +from milatools.utils.compute_node import ComputeNode +from milatools.utils.local_v2 import LocalV2 +from milatools.utils.remote_v2 import RemoteV2, UnsupportedPlatformError + + +@pytest.fixture +def pretend_to_be_in_WSL( + request: pytest.FixtureRequest, monkeypatch: pytest.MonkeyPatch +): + # By default, pretend to be in WSL. Indirect parametrization can be used to + # overwrite this value for a given test (as is done below). + in_wsl = getattr(request, "param", True) + + _mock_running_inside_WSL = Mock(spec=running_inside_WSL, return_value=in_wsl) + monkeypatch.setattr( + milatools.cli.utils, + running_inside_WSL.__name__, # type: ignore + _mock_running_inside_WSL, + ) + monkeypatch.setattr( + milatools.cli.code, + running_inside_WSL.__name__, # type: ignore + _mock_running_inside_WSL, + ) + return in_wsl + + +@pytest.mark.xfail( + sys.platform == "win32", + raises=UnsupportedPlatformError, + reason="Uses RemoteV2, so isn't supported on Windows.", + strict=True, +) +@pytest.mark.parametrize("pretend_to_be_in_WSL", [True, False], indirect=True) +@pytest.mark.asyncio +async def test_code_from_WSL( + monkeypatch: pytest.MonkeyPatch, pretend_to_be_in_WSL: bool +): + # Don't actually connect to the cluster. + monkeypatch.setattr(milatools.cli.code, RemoteV2.__name__, Mock(spec=RemoteV2)) + + # Mock the LocalV2 class so that we can inspect the call to `LocalV2.run_async`. + mock_localv2 = Mock(spec=LocalV2) + monkeypatch.setattr(milatools.cli.code, LocalV2.__name__, mock_localv2) + + await milatools.cli.code.launch_vscode_loop( + "code", Mock(spec=ComputeNode, hostname="foo"), "/bob/path" + ) + assert isinstance(mock_localv2.run_async, AsyncMock) + mock_localv2.run_async.assert_called_once_with( + ( + *(("powershell.exe",) if pretend_to_be_in_WSL else ()), + "code", + "--new-window", + "--wait", + "--remote", + "ssh-remote+foo", + "/bob/path", + ), + display=True, + ) From 5ff4bbf830f6ef39da258667c8256690c55948c5 Mon Sep 17 00:00:00 2001 From: Fabrice Normandin Date: Fri, 11 Oct 2024 11:49:57 -0400 Subject: [PATCH 2/7] Bugfix for mila code in WSL Signed-off-by: Fabrice Normandin --- milatools/cli/code.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/milatools/cli/code.py b/milatools/cli/code.py index 2c9ef573..47615d3e 100644 --- a/milatools/cli/code.py +++ b/milatools/cli/code.py @@ -13,6 +13,7 @@ MilatoolsUserError, currently_in_a_test, internet_on_compute_nodes, + running_inside_WSL, ) from milatools.utils.compute_node import ComputeNode, salloc, sbatch from milatools.utils.disk_quota import check_disk_quota @@ -193,6 +194,9 @@ async def launch_vscode_loop(code_command: str, compute_node: ComputeNode, path: f"ssh-remote+{compute_node.hostname}", path, ) + if running_inside_WSL(): + code_command_to_run = ("powershell.exe", *code_command_to_run) + await LocalV2.run_async(code_command_to_run, display=True) # TODO: BUG: This now requires two Ctrl+C's instead of one! console.print( From fe8452b55de4ccecc2b3bdc3ad2cdcadad065fc0 Mon Sep 17 00:00:00 2001 From: Fabrice Normandin Date: Fri, 11 Oct 2024 11:53:47 -0400 Subject: [PATCH 3/7] Fix issue in pre-commit hook - https://github.com/PyCQA/docformatter/issues/289 - https://github.com/PyCQA/docformatter/pull/287 Signed-off-by: Fabrice Normandin --- .github/workflows/build.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index e8b5b4a0..2280b70f 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -25,7 +25,7 @@ jobs: - uses: actions/setup-python@v4 with: python-version: '3.10' - - run: pip install pre-commit + - run: pip install "pre-commit<4.0.0" - run: pre-commit --version - run: pre-commit install - run: pre-commit run --all-files From ec41ced1290599edb25ec96a18ed1ae4b0988b47 Mon Sep 17 00:00:00 2001 From: Fabrice Normandin Date: Fri, 11 Oct 2024 12:02:04 -0400 Subject: [PATCH 4/7] Remove unnecessary mock for RemoteV2 in test Signed-off-by: Fabrice Normandin --- tests/cli/test_code.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/cli/test_code.py b/tests/cli/test_code.py index bbc236b2..57b2d148 100644 --- a/tests/cli/test_code.py +++ b/tests/cli/test_code.py @@ -14,7 +14,7 @@ from milatools.cli.utils import running_inside_WSL from milatools.utils.compute_node import ComputeNode from milatools.utils.local_v2 import LocalV2 -from milatools.utils.remote_v2 import RemoteV2, UnsupportedPlatformError +from milatools.utils.remote_v2 import UnsupportedPlatformError @pytest.fixture @@ -50,9 +50,6 @@ def pretend_to_be_in_WSL( async def test_code_from_WSL( monkeypatch: pytest.MonkeyPatch, pretend_to_be_in_WSL: bool ): - # Don't actually connect to the cluster. - monkeypatch.setattr(milatools.cli.code, RemoteV2.__name__, Mock(spec=RemoteV2)) - # Mock the LocalV2 class so that we can inspect the call to `LocalV2.run_async`. mock_localv2 = Mock(spec=LocalV2) monkeypatch.setattr(milatools.cli.code, LocalV2.__name__, mock_localv2) From 439a2531c1f7b30c5eb6aceb3b5e2c001179231a Mon Sep 17 00:00:00 2001 From: Fabrice Normandin Date: Fri, 11 Oct 2024 12:30:21 -0400 Subject: [PATCH 5/7] Remove unnecessary xfail mark on Windows Signed-off-by: Fabrice Normandin --- tests/cli/test_code.py | 8 -------- 1 file changed, 8 deletions(-) diff --git a/tests/cli/test_code.py b/tests/cli/test_code.py index 57b2d148..4e09500d 100644 --- a/tests/cli/test_code.py +++ b/tests/cli/test_code.py @@ -4,7 +4,6 @@ moved here, since some of them aren't exactly "integration" tests. """ -import sys from unittest.mock import AsyncMock, Mock import pytest @@ -14,7 +13,6 @@ from milatools.cli.utils import running_inside_WSL from milatools.utils.compute_node import ComputeNode from milatools.utils.local_v2 import LocalV2 -from milatools.utils.remote_v2 import UnsupportedPlatformError @pytest.fixture @@ -39,12 +37,6 @@ def pretend_to_be_in_WSL( return in_wsl -@pytest.mark.xfail( - sys.platform == "win32", - raises=UnsupportedPlatformError, - reason="Uses RemoteV2, so isn't supported on Windows.", - strict=True, -) @pytest.mark.parametrize("pretend_to_be_in_WSL", [True, False], indirect=True) @pytest.mark.asyncio async def test_code_from_WSL( From 7be5c30df0c77cea3f7d547b89819bbe9ce9e3de Mon Sep 17 00:00:00 2001 From: Fabrice Normandin Date: Fri, 11 Oct 2024 13:59:42 -0400 Subject: [PATCH 6/7] Adjust the regression test files following changes Signed-off-by: Fabrice Normandin --- tests/integration/test_code/test_code_mila__salloc_.txt | 2 +- tests/integration/test_code/test_code_mila__sbatch_.txt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/test_code/test_code_mila__salloc_.txt b/tests/integration/test_code/test_code_mila__salloc_.txt index 14a6e463..053d4abc 100644 --- a/tests/integration/test_code/test_code_mila__salloc_.txt +++ b/tests/integration/test_code/test_code_mila__salloc_.txt @@ -2,7 +2,7 @@ Checking disk quota on $HOME... Disk usage: X / LIMIT GiB and X / LIMIT files (mila) $ cd $SCRATCH && salloc --wckey=milatools_test --account=SLURM_ACCOUNT --nodes=1 --ntasks=1 --cpus-per-task=1 --mem=1G --time=0:05:00 --oversubscribe --job-name=mila-code salloc: -------------------------------------------------------------------------------------------------- -salloc: # Using default long partition +salloc: # Using default long-cpu partition (CPU-only) salloc: -------------------------------------------------------------------------------------------------- salloc: Granted job allocation JOB_ID Waiting for job JOB_ID to start. diff --git a/tests/integration/test_code/test_code_mila__sbatch_.txt b/tests/integration/test_code/test_code_mila__sbatch_.txt index 249496de..b980ae67 100644 --- a/tests/integration/test_code/test_code_mila__sbatch_.txt +++ b/tests/integration/test_code/test_code_mila__sbatch_.txt @@ -5,7 +5,7 @@ Disk usage: X / LIMIT GiB and X / LIMIT files JOB_ID sbatch: -------------------------------------------------------------------------------------------------- -sbatch: # Using default long partition +sbatch: # Using default long-cpu partition (CPU-only) sbatch: -------------------------------------------------------------------------------------------------- (localhost) $ echo --new-window --wait --remote ssh-remote+COMPUTE_NODE $HOME/bob From 029e9b28f6fb3fd788ae519fbd74b5510a7d4f70 Mon Sep 17 00:00:00 2001 From: Fabrice Normandin Date: Fri, 11 Oct 2024 14:32:43 -0400 Subject: [PATCH 7/7] Add xfail for flaky integration test Signed-off-by: Fabrice Normandin --- tests/integration/test_sync_command.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/integration/test_sync_command.py b/tests/integration/test_sync_command.py index 31f19b4a..36ae27bd 100644 --- a/tests/integration/test_sync_command.py +++ b/tests/integration/test_sync_command.py @@ -19,6 +19,7 @@ _install_vscode_extensions_task_function, sync_vscode_extensions, ) +from tests.integration.conftest import SLURM_CLUSTER from ..cli.common import ( requires_ssh_to_localhost, @@ -28,6 +29,10 @@ logger = get_logger(__name__) +@pytest.mark.xfail( + SLURM_CLUSTER == "mila", + reason="`code-server` procs are killed on the login nodes of the Mila cluster.", +) @pytest.mark.slow @pytest.mark.parametrize( "source",