From 49a07ba2f5870d55a6c70f89e100fd8adc5a327b Mon Sep 17 00:00:00 2001 From: Anton Schwaighofer Date: Mon, 28 Mar 2022 15:12:37 +0100 Subject: [PATCH 01/23] pytest ini --- hi-ml-azure/pytest.ini | 1 + hi-ml-histopathology/pytest.ini | 1 + hi-ml/pytest.ini | 1 + 3 files changed, 3 insertions(+) diff --git a/hi-ml-azure/pytest.ini b/hi-ml-azure/pytest.ini index fa2eade28..a82601d75 100644 --- a/hi-ml-azure/pytest.ini +++ b/hi-ml-azure/pytest.ini @@ -8,3 +8,4 @@ markers = fast: Tests that should run very fast, and can act as smoke tests to see if something goes terribly wrong. slow: Tests that are slow to run and not crucial to the build. timeout: Tests will terminate and fail if not completed within this length of time. + gpu: Tests that require at least 1 GPU to be present diff --git a/hi-ml-histopathology/pytest.ini b/hi-ml-histopathology/pytest.ini index 9a6e19f71..3a848af86 100644 --- a/hi-ml-histopathology/pytest.ini +++ b/hi-ml-histopathology/pytest.ini @@ -6,3 +6,4 @@ log_cli_level = DEBUG addopts = --strict-markers markers = fast: Tests that should run very fast, and can act as smoke tests to see if something goes terribly wrong. + gpu: Tests that require at least 1 GPU to be present diff --git a/hi-ml/pytest.ini b/hi-ml/pytest.ini index 29f0a5878..e90f4e931 100644 --- a/hi-ml/pytest.ini +++ b/hi-ml/pytest.ini @@ -6,3 +6,4 @@ log_cli_level = DEBUG addopts = --strict-markers markers = fast: Tests that should run very fast, and can act as smoke tests to see if something goes terribly wrong. + gpu: Tests that require at least 1 GPU to be present From d5aededb96d2844d3c657dafcc06804c65546696 Mon Sep 17 00:00:00 2001 From: Anton Schwaighofer Date: Tue, 29 Mar 2022 10:11:16 +0100 Subject: [PATCH 02/23] better progress report --- hi-ml-azure/src/health_azure/himl.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/hi-ml-azure/src/health_azure/himl.py b/hi-ml-azure/src/health_azure/himl.py index a1950793b..cb8a06e9b 100644 --- a/hi-ml-azure/src/health_azure/himl.py +++ b/hi-ml-azure/src/health_azure/himl.py @@ -445,16 +445,17 @@ def submit_to_azure_if_needed( # type: ignore ) if snapshot_root_directory is None: - logging.info(f"No snapshot root directory given. Uploading all files in the current directory {Path.cwd()}") + print(f"No snapshot root directory given. Uploading all files in the current directory {Path.cwd()}") snapshot_root_directory = Path.cwd() workspace = get_workspace(aml_workspace, workspace_config_path) + print(f"Loaded AzureML workspace {workspace.name}") if conda_environment_file is None: conda_environment_file = find_file_in_parent_to_pythonpath(CONDA_ENVIRONMENT_FILE) + print(f"Using the Conda environment from this file: {conda_environment_file}") conda_environment_file = _str_to_path(conda_environment_file) - logging.info(f"Loaded AzureML workspace {workspace.name}") run_config = create_run_configuration( workspace=workspace, compute_cluster_name=compute_cluster_name, From 31378103f32dddd0d63bb1ae24e57038fb014e5b Mon Sep 17 00:00:00 2001 From: Anton Schwaighofer Date: Tue, 29 Mar 2022 10:12:09 +0100 Subject: [PATCH 03/23] ignore --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 75adf043d..baeca7a49 100644 --- a/.gitignore +++ b/.gitignore @@ -165,3 +165,4 @@ cifar-10-batches-py/ None/ hi-ml-histopathology/testSSL/test_outputs test_outputs/ +lightning_logs/ From 3beef2671cbaf265306c3c49074803773992ec6b Mon Sep 17 00:00:00 2001 From: Anton Schwaighofer Date: Tue, 29 Mar 2022 10:13:52 +0100 Subject: [PATCH 04/23] color --- .vscode/settings.json | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.vscode/settings.json b/.vscode/settings.json index ca2f6bdec..1df60e0ba 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -72,5 +72,10 @@ }, "terminal.integrated.env.linux": { "PYTHONPATH":"${workspaceFolder}/hi-ml/src:${workspaceFolder}/hi-ml-azure/src:${workspaceFolder}/hi-ml-histopathology/src" + }, + "workbench.colorCustomizations": { + "activityBar.background": "#077203", + "titleBar.activeBackground": "#077203", + "titleBar.activeForeground": "#FBFAF6" } } \ No newline at end of file From b1bfb83f8a38712aa455ae2c42ee479f4839086d Mon Sep 17 00:00:00 2001 From: Anton Schwaighofer Date: Tue, 29 Mar 2022 10:14:09 +0100 Subject: [PATCH 05/23] pytest runner --- .vscode/launch.json | 14 +++++ hi-ml-histopathology/environment.yml | 2 + hi-ml/run_pytest.py | 82 ++++++++++++++++++++++++++++ 3 files changed, 98 insertions(+) create mode 100644 hi-ml/run_pytest.py diff --git a/.vscode/launch.json b/.vscode/launch.json index d23676e6e..c37243847 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -46,6 +46,20 @@ ], "console": "integratedTerminal" }, + { + "name": "Python: Run pytest for GPU tests only", + "type": "python", + "request": "launch", + "program": "${workspaceFolder}/hi-ml/run_pytest.py", + "cwd": "${workspaceFolder}", + "args": [ + "--mark=gpu", + "--azureml", + "--cluster=pr-gpu", + "--conda_env=${workspaceFolder}/hi-ml-histopathology/environment.yml", + ], + "console": "integratedTerminal" + }, { "name": "Python: Run DeepSMILECrck in AzureML", "type": "python", diff --git a/hi-ml-histopathology/environment.yml b/hi-ml-histopathology/environment.yml index 84a57ad0a..fc090b895 100644 --- a/hi-ml-histopathology/environment.yml +++ b/hi-ml-histopathology/environment.yml @@ -56,3 +56,5 @@ dependencies: - types-python-dateutil==2.8.9 - umap-learn==0.5.2 - yacs==0.1.8 + # Test requirements + - pytest==6.2.2 diff --git a/hi-ml/run_pytest.py b/hi-ml/run_pytest.py new file mode 100644 index 000000000..c9e61d9e7 --- /dev/null +++ b/hi-ml/run_pytest.py @@ -0,0 +1,82 @@ +import logging +import sys +from pathlib import Path +from typing import Tuple + +import pytest +import param +from _pytest.main import ExitCode + +# Add hi-ml packages to sys.path so that AML can find them if we are using the runner directly from the git repo +himl_root = Path(__file__).resolve().parent.parent +folders_to_add = [himl_root / "hi-ml" / "src", himl_root / "hi-ml-azure" / "src"] +for folder in folders_to_add: + folder_str = str(folder) + if folder.is_dir() and folder_str not in sys.path: + sys.path.insert(0, str(folder)) + +from health_azure import submit_to_azure_if_needed # noqa: E402 +from health_azure.utils import create_argparser, parse_arguments # noqa: E402 +from health_ml.utils.common_utils import DEFAULT_AML_UPLOAD_DIR # noqa: E402 +from health_ml.utils.fixed_paths import repository_root_directory # noqa: E402 + +PYTEST_RESULTS_FILE = "pytest_results.xml" + + +class RunPytestConfig(param.Parameterized): + mark: str = param.String(default="", doc="The value to pass to pytest for the -m (mark) argument.") + folder: str = param.String(default="", doc="The folder of tests that should be run.") + cluster: str = param.String(default="", doc="The name of the AzureML compute cluster where the script should run.") + azureml: bool = param.Boolean(default=False, doc="If True, submit the present script to AzureML.") + conda_env: str = param.String(default="", doc="The path to the Conda environment file that should be used.") + experiment: str = param.String(default="", doc="The name of the AzureML experiment where the run should live.") + + +def run_pytest(folder: str, pytest_mark: str) -> Tuple[bool, Path]: + """ + Runs pytest on a given folder, restricting to the tests that have the given PyTest mark. + + :param pytest_mark: The PyTest mark to use for filtering out the tests to run. + :param outputs_folder: The folder into which the test result XML file should be written. + :return: True if PyTest found tests to execute and completed successfully, False otherwise. + Also returns the path to the generated PyTest results file. + """ + results_file = Path(DEFAULT_AML_UPLOAD_DIR) / PYTEST_RESULTS_FILE + pytest_args = [folder, f"--junitxml={str(results_file)}"] + + if pytest_mark: + pytest_args += ["-m", pytest_mark] + logging.info(f"Starting pytest, with args: {pytest_args}") + status_code = pytest.main(pytest_args) + if status_code == ExitCode.NO_TESTS_COLLECTED: + raise ValueError(f"PyTest did not find any tests to run, when restricting with this mark: {pytest_mark}") + if status_code != ExitCode.OK: + raise ValueError(f"PyTest failed with exit code: {status_code}") + + +def logging_to_stdout() -> None: + """Redirects output from the logging module to stdout.""" + root = logging.getLogger() + root.setLevel(logging.DEBUG) + handler = logging.StreamHandler(sys.stdout) + handler.setLevel(logging.INFO) + root.addHandler(handler) + + +if __name__ == "__main__": + logging_to_stdout() + + config = RunPytestConfig() + + parser = create_argparser(config) + parser_results = parse_arguments(parser, fail_on_unknown_args=True) + config = RunPytestConfig(**parser_results.args) + submit_to_azure_if_needed( + compute_cluster_name=config.cluster, + submit_to_azureml=config.azureml, + wait_for_completion=True, + snapshot_root_directory=repository_root_directory(), + conda_environment_file=config.conda_env, + experiment_name=config.experiment, + ) + run_pytest(folder=config.folder, pytest_mark=config.mark) From daedd2753325477fbefdf292f429d11586b5fb28 Mon Sep 17 00:00:00 2001 From: Anton Schwaighofer Date: Tue, 29 Mar 2022 10:19:28 +0100 Subject: [PATCH 06/23] adding gpu tests --- hi-ml-histopathology/testSSL/testSSL/test_ssl_containers.py | 2 ++ .../testhisto/testhisto/utils/test_output_utils.py | 2 ++ 2 files changed, 4 insertions(+) diff --git a/hi-ml-histopathology/testSSL/testSSL/test_ssl_containers.py b/hi-ml-histopathology/testSSL/testSSL/test_ssl_containers.py index a49c1f915..f330968a0 100644 --- a/hi-ml-histopathology/testSSL/testSSL/test_ssl_containers.py +++ b/hi-ml-histopathology/testSSL/testSSL/test_ssl_containers.py @@ -308,6 +308,7 @@ def test_simclr_lr_scheduler() -> None: @pytest.mark.skipif(no_gpu, reason="Test requires GPU") +@pytest.mark.gpu def test_simclr_training_recovery(test_output_dirs: OutputFolderForTests) -> None: """ This test checks if a SSLContainer correctly resumes training. First we run SSL using a Trainer for 20 epochs. @@ -447,6 +448,7 @@ def test_online_evaluator_recovery(test_output_dirs: OutputFolderForTests) -> No @pytest.mark.skipif(no_gpu, reason="Test requires GPU") +@pytest.mark.gpu def test_online_evaluator_not_distributed() -> None: """ Check if the online evaluator uses the DDP flag correctly when running not distributed diff --git a/hi-ml-histopathology/testhisto/testhisto/utils/test_output_utils.py b/hi-ml-histopathology/testhisto/testhisto/utils/test_output_utils.py index a7fd9859b..17a79f9df 100644 --- a/hi-ml-histopathology/testhisto/testhisto/utils/test_output_utils.py +++ b/hi-ml-histopathology/testhisto/testhisto/utils/test_output_utils.py @@ -155,6 +155,7 @@ def save_validation_outputs(handler: DeepMILOutputsHandler, metric_value: float, @pytest.mark.skipif(not torch.distributed.is_available(), reason="PyTorch distributed unavailable") @pytest.mark.skipif(torch.cuda.device_count() < 2, reason="Not enough GPUs available") +@pytest.mark.gpu def test_overwriting_val_outputs_distributed(tmp_path: Path) -> None: run_distributed(test_overwriting_val_outputs, args=(tmp_path,), world_size=2) @@ -207,6 +208,7 @@ def test_gather_results(rank: int = 0, world_size: int = 1, device: str = 'cpu') @pytest.mark.skipif(not torch.distributed.is_available(), reason="PyTorch distributed unavailable") @pytest.mark.skipif(torch.cuda.device_count() < 2, reason="Not enough GPUs available") +@pytest.mark.gpu def test_gather_results_distributed() -> None: # These tests need to be called sequentially to prevent them to be run in parallel run_distributed(test_gather_results, world_size=1) From 09f059b296da0bc9db20ba943c5b831a99b4937e Mon Sep 17 00:00:00 2001 From: Anton Schwaighofer Date: Tue, 29 Mar 2022 11:07:08 +0100 Subject: [PATCH 07/23] Add to PR build --- .github/workflows/build-test-pr.yml | 5 +++++ hi-ml/run_pytest.py | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/.github/workflows/build-test-pr.yml b/.github/workflows/build-test-pr.yml index 5b8fb3a73..591e9c6a9 100644 --- a/.github/workflows/build-test-pr.yml +++ b/.github/workflows/build-test-pr.yml @@ -258,6 +258,11 @@ jobs: make pip_local make pytest_and_coverage + - name: Run GPU tests + run: | + python hi-ml/run_pytest.py --mark=gpu --cluster=pr-gpu --conda_env=${{ matrix.folder }}/environment.yml --folder=${{ matrix.folder }} + + - name: Upload coverage artifacts uses: ./.github/actions/upload_coverage_artifacts if: ${{ matrix.packageName == '*.whl' }} diff --git a/hi-ml/run_pytest.py b/hi-ml/run_pytest.py index c9e61d9e7..86528110b 100644 --- a/hi-ml/run_pytest.py +++ b/hi-ml/run_pytest.py @@ -46,7 +46,7 @@ def run_pytest(folder: str, pytest_mark: str) -> Tuple[bool, Path]: if pytest_mark: pytest_args += ["-m", pytest_mark] - logging.info(f"Starting pytest, with args: {pytest_args}") + logging.info(f"Starting pytest with args: {pytest_args}") status_code = pytest.main(pytest_args) if status_code == ExitCode.NO_TESTS_COLLECTED: raise ValueError(f"PyTest did not find any tests to run, when restricting with this mark: {pytest_mark}") From 5df4260da5d1a672492b540141e8cd88914a2f03 Mon Sep 17 00:00:00 2001 From: Anton Schwaighofer Date: Tue, 29 Mar 2022 11:27:51 +0100 Subject: [PATCH 08/23] simplify args --- .vscode/launch.json | 1 - hi-ml/run_pytest.py | 5 ++--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index c37243847..d8396166f 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -54,7 +54,6 @@ "cwd": "${workspaceFolder}", "args": [ "--mark=gpu", - "--azureml", "--cluster=pr-gpu", "--conda_env=${workspaceFolder}/hi-ml-histopathology/environment.yml", ], diff --git a/hi-ml/run_pytest.py b/hi-ml/run_pytest.py index 86528110b..fad71cc45 100644 --- a/hi-ml/run_pytest.py +++ b/hi-ml/run_pytest.py @@ -27,7 +27,6 @@ class RunPytestConfig(param.Parameterized): mark: str = param.String(default="", doc="The value to pass to pytest for the -m (mark) argument.") folder: str = param.String(default="", doc="The folder of tests that should be run.") cluster: str = param.String(default="", doc="The name of the AzureML compute cluster where the script should run.") - azureml: bool = param.Boolean(default=False, doc="If True, submit the present script to AzureML.") conda_env: str = param.String(default="", doc="The path to the Conda environment file that should be used.") experiment: str = param.String(default="", doc="The name of the AzureML experiment where the run should live.") @@ -46,7 +45,7 @@ def run_pytest(folder: str, pytest_mark: str) -> Tuple[bool, Path]: if pytest_mark: pytest_args += ["-m", pytest_mark] - logging.info(f"Starting pytest with args: {pytest_args}") + logging.info(f"Starting pytest with these args: {pytest_args}") status_code = pytest.main(pytest_args) if status_code == ExitCode.NO_TESTS_COLLECTED: raise ValueError(f"PyTest did not find any tests to run, when restricting with this mark: {pytest_mark}") @@ -73,7 +72,7 @@ def logging_to_stdout() -> None: config = RunPytestConfig(**parser_results.args) submit_to_azure_if_needed( compute_cluster_name=config.cluster, - submit_to_azureml=config.azureml, + submit_to_azureml=(config.cluster != ""), wait_for_completion=True, snapshot_root_directory=repository_root_directory(), conda_environment_file=config.conda_env, From b78af8a68c0cd37ee80b0c9a36f8447bb43db0dd Mon Sep 17 00:00:00 2001 From: Anton Schwaighofer Date: Tue, 29 Mar 2022 16:04:05 +0100 Subject: [PATCH 09/23] fix experiment name --- .github/workflows/build-test-pr.yml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/.github/workflows/build-test-pr.yml b/.github/workflows/build-test-pr.yml index 591e9c6a9..2943f132a 100644 --- a/.github/workflows/build-test-pr.yml +++ b/.github/workflows/build-test-pr.yml @@ -260,8 +260,10 @@ jobs: - name: Run GPU tests run: | - python hi-ml/run_pytest.py --mark=gpu --cluster=pr-gpu --conda_env=${{ matrix.folder }}/environment.yml --folder=${{ matrix.folder }} - + branch_prefix="refs/heads/" + full_branch_name=$GITHUB_REF + branch_name_without_prefix=${full_branch_name#$branch_prefix} + python hi-ml/run_pytest.py --mark=gpu --cluster=pr-gpu --conda_env=${{ matrix.folder }}/environment.yml --folder=${{ matrix.folder }} --experiment="$branch_name_without_prefix" - name: Upload coverage artifacts uses: ./.github/actions/upload_coverage_artifacts From 48e00ccc3dab66d272eaec93bb3e63ba4ab32108 Mon Sep 17 00:00:00 2001 From: Anton Schwaighofer Date: Tue, 29 Mar 2022 16:17:27 +0100 Subject: [PATCH 10/23] pyright and doc --- hi-ml/run_pytest.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/hi-ml/run_pytest.py b/hi-ml/run_pytest.py index fad71cc45..20b176a3a 100644 --- a/hi-ml/run_pytest.py +++ b/hi-ml/run_pytest.py @@ -31,17 +31,17 @@ class RunPytestConfig(param.Parameterized): experiment: str = param.String(default="", doc="The name of the AzureML experiment where the run should live.") -def run_pytest(folder: str, pytest_mark: str) -> Tuple[bool, Path]: +def run_pytest(folder_to_test: str, pytest_mark: str) -> None: """ Runs pytest on a given folder, restricting to the tests that have the given PyTest mark. + If pytest finds no tests, or any of the tests fail, this function raises a ValueError. When run inside + AzureML, this will make the job fail. :param pytest_mark: The PyTest mark to use for filtering out the tests to run. - :param outputs_folder: The folder into which the test result XML file should be written. - :return: True if PyTest found tests to execute and completed successfully, False otherwise. - Also returns the path to the generated PyTest results file. + :param folder_to_test: The folder with tests that should be run. """ results_file = Path(DEFAULT_AML_UPLOAD_DIR) / PYTEST_RESULTS_FILE - pytest_args = [folder, f"--junitxml={str(results_file)}"] + pytest_args = [folder_to_test, f"--junitxml={str(results_file)}"] if pytest_mark: pytest_args += ["-m", pytest_mark] @@ -78,4 +78,4 @@ def logging_to_stdout() -> None: conda_environment_file=config.conda_env, experiment_name=config.experiment, ) - run_pytest(folder=config.folder, pytest_mark=config.mark) + run_pytest(folder_to_test=config.folder, pytest_mark=config.mark) From 5cbca61cae3853cd85ec235ad5715231d3158aa6 Mon Sep 17 00:00:00 2001 From: Anton Schwaighofer Date: Tue, 29 Mar 2022 16:53:16 +0100 Subject: [PATCH 11/23] cleanup and add config.json --- hi-ml-azure/src/health_azure/utils.py | 45 +++++++++++++++- hi-ml-azure/testazure/testazure/test_himl.py | 14 ++--- .../testazure/testazure/utils_testazure.py | 47 ++-------------- hi-ml-histopathology/testSSL/testSSL/utils.py | 53 ------------------- .../testhisto/utils/utils_testhisto.py | 12 ----- hi-ml/run_pytest.py | 24 +++++---- 6 files changed, 69 insertions(+), 126 deletions(-) diff --git a/hi-ml-azure/src/health_azure/utils.py b/hi-ml-azure/src/health_azure/utils.py index c62949315..be55d3511 100644 --- a/hi-ml-azure/src/health_azure/utils.py +++ b/hi-ml-azure/src/health_azure/utils.py @@ -5,6 +5,7 @@ """ Utility functions for interacting with AzureML runs """ +from contextlib import contextmanager import hashlib import json import logging @@ -27,7 +28,7 @@ from enum import Enum from itertools import islice from pathlib import Path -from typing import Any, Callable, DefaultDict, Dict, List, Optional, Set, Tuple, Type, TypeVar, Union +from typing import Any, Callable, DefaultDict, Dict, Generator, List, Optional, Set, Tuple, Type, TypeVar, Union import conda_merge import pandas as pd @@ -1974,3 +1975,45 @@ def workspace(self) -> Workspace: if self._workspace is None: self._workspace = aml_workspace_for_unittests() return self._workspace + + +@contextmanager +def check_config_json(script_folder: Path, shared_config_json: Path) -> Generator: + """ + Create a workspace config.json file exists in the folder where we expect a test script. This is either copied + from the location given in shared_config_json (this should be the case when executing a test on a dev machine), + or created from environment variables (this should trigger in builds on the github agents). + + :param script_folder: This is the folder in which the config.json file should be created + :param shared_config_json: Path to a shared config.json file + """ + target_config_json = script_folder / WORKSPACE_CONFIG_JSON + target_config_exists = target_config_json.is_file() + if target_config_exists: + pass + elif shared_config_json.exists(): + # This will execute on local dev machines + logging.info(f"Copying {shared_config_json} to folder {script_folder}") + shutil.copy(shared_config_json, target_config_json) + else: + # This will execute on github agents + logging.info(f"Creating {str(target_config_json)} from environment variables.") + subscription_id = os.getenv(ENV_SUBSCRIPTION_ID, "") + resource_group = os.getenv(ENV_RESOURCE_GROUP, "") + workspace_name = os.getenv(ENV_WORKSPACE_NAME, "") + if subscription_id and resource_group and workspace_name: + with open(str(target_config_json), 'w', encoding="utf-8") as file: + config = { + "subscription_id": os.getenv(ENV_SUBSCRIPTION_ID, ""), + "resource_group": os.getenv(ENV_RESOURCE_GROUP, ""), + "workspace_name": os.getenv(ENV_WORKSPACE_NAME, "") + } + json.dump(config, file) + else: + raise ValueError("Either a shared config.json must be present, or all 3 environment variables for " + "workspace creation must exist.") + try: + yield + finally: + if not target_config_exists: + target_config_json.unlink() diff --git a/hi-ml-azure/testazure/testazure/test_himl.py b/hi-ml-azure/testazure/testazure/test_himl.py index 233669636..350084c58 100644 --- a/hi-ml-azure/testazure/testazure/test_himl.py +++ b/hi-ml-azure/testazure/testazure/test_himl.py @@ -32,9 +32,9 @@ import health_azure.himl as himl from health_azure.datasets import (DatasetConfig, _input_dataset_key, _output_dataset_key, get_datastore) from health_azure.utils import (ENVIRONMENT_VERSION, EXPERIMENT_RUN_SEPARATOR, WORKSPACE_CONFIG_JSON, - get_most_recent_run, get_workspace, is_running_in_azure_ml) + check_config_json, get_most_recent_run, get_workspace, is_running_in_azure_ml) from testazure.test_data.make_tests import render_environment_yaml, render_test_script -from testazure.utils_testazure import DEFAULT_DATASTORE, change_working_directory, check_config_json, repository_root +from testazure.utils_testazure import DEFAULT_DATASTORE, change_working_directory, get_shared_config_json, repository_root INEXPENSIVE_TESTING_CLUSTER_NAME = "lite-testing-ds2" EXPECTED_QUEUED = "This command will be run in AzureML:" @@ -694,7 +694,7 @@ def spawn() -> Tuple[int, List[str]]: if suppress_config_creation: code, stdout = spawn() else: - with check_config_json(path): + with check_config_json(path, shared_config_json=get_shared_config_json()): code, stdout = spawn() assert code == 0 if expected_pass else 1, f"Expected the script to {'pass' if expected_pass else 'fail'}, but " \ f"got a return code {code}" @@ -705,7 +705,7 @@ def spawn() -> Tuple[int, List[str]]: return captured else: assert EXPECTED_QUEUED in captured - with check_config_json(path): + with check_config_json(path, shared_config_json=get_shared_config_json()): workspace = get_workspace(aml_workspace=None, workspace_config_path=path / WORKSPACE_CONFIG_JSON) run = get_most_recent_run(run_recovery_file=path / himl.RUN_RECOVERY_FILE, @@ -847,7 +847,7 @@ def test_invoking_hello_world_env_var(run_target: RunTarget, tmp_path: Path) -> @pytest.mark.timeout(300) def test_mounting_dataset(tmp_path: Path) -> None: logging.info("creating config.json") - with check_config_json(tmp_path): + with check_config_json(tmp_path, shared_config_json=get_shared_config_json()): logging.info("get_workspace") workspace = get_workspace(aml_workspace=None, workspace_config_path=tmp_path / WORKSPACE_CONFIG_JSON) @@ -872,7 +872,7 @@ def test_mounting_dataset(tmp_path: Path) -> None: @pytest.mark.timeout(60) def test_downloading_dataset(tmp_path: Path) -> None: logging.info("creating config.json") - with check_config_json(tmp_path): + with check_config_json(tmp_path, shared_config_json=get_shared_config_json()): logging.info("get_workspace") workspace = get_workspace(aml_workspace=None, workspace_config_path=tmp_path / WORKSPACE_CONFIG_JSON) @@ -974,7 +974,7 @@ def test_invoking_hello_world_datasets(run_target: RunTarget, for i in range(0, output_count)] # Get default datastore - with check_config_json(tmp_path): + with check_config_json(tmp_path, shared_config_json=get_shared_config_json()): workspace = get_workspace(aml_workspace=None, workspace_config_path=tmp_path / WORKSPACE_CONFIG_JSON) datastore: AzureBlobDatastore = get_datastore(workspace=workspace, diff --git a/hi-ml-azure/testazure/testazure/utils_testazure.py b/hi-ml-azure/testazure/testazure/utils_testazure.py index 2e3e2551c..3c51d1a3f 100644 --- a/hi-ml-azure/testazure/testazure/utils_testazure.py +++ b/hi-ml-azure/testazure/testazure/utils_testazure.py @@ -5,16 +5,12 @@ """ Test utility functions for tests in the package. """ -import json -import logging import os -import shutil from contextlib import contextmanager from pathlib import Path from typing import Dict, Generator, Optional -from health_azure.utils import (ENV_RESOURCE_GROUP, ENV_SUBSCRIPTION_ID, ENV_WORKSPACE_NAME, WORKSPACE_CONFIG_JSON, - UnitTestWorkspaceWrapper) +from health_azure.utils import (UnitTestWorkspaceWrapper, WORKSPACE_CONFIG_JSON) DEFAULT_DATASTORE = "himldatasets" FALLBACK_SINGLE_RUN = "refs_pull_545_merge:refs_pull_545_merge_1626538212_d2b07afd" @@ -23,6 +19,8 @@ DEFAULT_IGNORE_FOLDERS = [".config", ".git", ".github", ".idea", ".mypy_cache", ".pytest_cache", ".vscode", "docs", "node_modules"] +DEFAULT_WORKSPACE = UnitTestWorkspaceWrapper() + class MockRun: def __init__(self, run_id: str = 'run1234', tags: Optional[Dict[str, str]] = None) -> None: @@ -48,9 +46,6 @@ def repository_root() -> Path: return himl_azure_root().parent -DEFAULT_WORKSPACE = UnitTestWorkspaceWrapper() - - @contextmanager def change_working_directory(path_or_str: Path) -> Generator: """ @@ -68,39 +63,3 @@ def get_shared_config_json() -> Path: Gets the path to the config.json file that should exist for running tests locally (outside github build agents). """ return repository_root() / "hi-ml-azure" / "testazure" / WORKSPACE_CONFIG_JSON - - -@contextmanager -def check_config_json(script_folder: Path) -> Generator: - """ - Create a workspace config.json file in the folder where we expect the test scripts. This is either copied - from the repository root folder (this should be the case when executing a test on a dev machine), or create - it from environment variables (this should trigger in builds on the github agents). - - :param script_folder: This is the folder in which the config.json file should be created - """ - shared_config_json = get_shared_config_json() - target_config_json = script_folder / WORKSPACE_CONFIG_JSON - if shared_config_json.exists(): - logging.info(f"Copying {WORKSPACE_CONFIG_JSON} from repository root to folder {script_folder}") - shutil.copy(shared_config_json, target_config_json) - else: - logging.info(f"Creating {str(target_config_json)} from environment variables.") - subscription_id = os.getenv(ENV_SUBSCRIPTION_ID, "") - resource_group = os.getenv(ENV_RESOURCE_GROUP, "") - workspace_name = os.getenv(ENV_WORKSPACE_NAME, "") - if subscription_id and resource_group and workspace_name: - with open(str(target_config_json), 'w', encoding="utf-8") as file: - config = { - "subscription_id": os.getenv(ENV_SUBSCRIPTION_ID, ""), - "resource_group": os.getenv(ENV_RESOURCE_GROUP, ""), - "workspace_name": os.getenv(ENV_WORKSPACE_NAME, "") - } - json.dump(config, file) - else: - raise ValueError("Either a shared config.json must be present, or all 3 environment variables for " - "workspace creation must exist.") - try: - yield - finally: - target_config_json.unlink() diff --git a/hi-ml-histopathology/testSSL/testSSL/utils.py b/hi-ml-histopathology/testSSL/testSSL/utils.py index c6464e6f3..9dcdcfb9b 100644 --- a/hi-ml-histopathology/testSSL/testSSL/utils.py +++ b/hi-ml-histopathology/testSSL/testSSL/utils.py @@ -25,18 +25,6 @@ TEST_OUTPUTS_PATH = Path(__file__).parent.parent / "test_outputs" -def tests_root_directory(path: Optional[PathOrString] = None) -> Path: - """ - Gets the full path to the root directory that holds the tests. - If a relative path is provided then concatenate it with the absolute path - to the repository root. - - :return: The full path to the repository's root directory, with symlinks resolved if any. - """ - root = Path(os.path.realpath(__file__)).parent.parent - return root / path if path else root - - def write_test_dicom(array: np.ndarray, path: Path, is_monochrome2: bool = True, bits_stored: Optional[int] = None) -> None: """ @@ -58,44 +46,3 @@ def write_test_dicom(array: np.ndarray, path: Path, is_monochrome2: bool = True, if bits_stored is not None: ds.BitsStored = bits_stored ds.save_as(path) - - -def get_shared_config_json() -> Path: - """ - Gets the path to the config.json file that should exist for running tests locally (outside github build agents). - """ - return tests_root_directory() / WORKSPACE_CONFIG_JSON - - -@contextmanager -def check_config_json(script_folder: Path) -> Generator: - """ - Create a workspace config.json file in the folder where we expect the test scripts. This is either copied - from the repository root folder (this should be the case when executing a test on a dev machine), or create - it from environment variables (this should trigger in builds on the github agents). - """ - shared_config_json = get_shared_config_json() - target_config_json = script_folder / WORKSPACE_CONFIG_JSON - if shared_config_json.exists(): - logging.info(f"Copying {WORKSPACE_CONFIG_JSON} from repository root to folder {script_folder}") - shutil.copy(shared_config_json, target_config_json) - else: - logging.info(f"Creating {str(target_config_json)} from environment variables.") - subscription_id = os.getenv(ENV_SUBSCRIPTION_ID, "") - resource_group = os.getenv(ENV_RESOURCE_GROUP, "") - workspace_name = os.getenv(ENV_WORKSPACE_NAME, "") - if subscription_id and resource_group and workspace_name: - with open(str(target_config_json), 'w', encoding="utf-8") as file: - config = { - "subscription_id": os.getenv(ENV_SUBSCRIPTION_ID, ""), - "resource_group": os.getenv(ENV_RESOURCE_GROUP, ""), - "workspace_name": os.getenv(ENV_WORKSPACE_NAME, "") - } - json.dump(config, file) - else: - raise ValueError("Either a shared config.json must be present, or all 3 environment variables for " - "workspace creation must exist.") - try: - yield - finally: - target_config_json.unlink() diff --git a/hi-ml-histopathology/testhisto/testhisto/utils/utils_testhisto.py b/hi-ml-histopathology/testhisto/testhisto/utils/utils_testhisto.py index 74a19a1e3..1db732287 100644 --- a/hi-ml-histopathology/testhisto/testhisto/utils/utils_testhisto.py +++ b/hi-ml-histopathology/testhisto/testhisto/utils/utils_testhisto.py @@ -14,18 +14,6 @@ from health_azure.utils import PathOrString -def tests_root_directory(path: Optional[PathOrString] = None) -> Path: - """ - Gets the full path to the root directory that holds the tests. - If a relative path is provided then concatenate it with the absolute path - to the repository root. - - :return: The full path to the repository's root directory, with symlinks resolved if any. - """ - root = Path(os.path.realpath(__file__)).parent.parent.parent - return root / path if path else root - - def assert_dicts_equal(d1: Mapping, d2: Mapping, exclude_keys: Collection[Any] = (), rtol: float = 1e-5, atol: float = 1e-8) -> None: assert isinstance(d1, Mapping) diff --git a/hi-ml/run_pytest.py b/hi-ml/run_pytest.py index 20b176a3a..140ba4f5f 100644 --- a/hi-ml/run_pytest.py +++ b/hi-ml/run_pytest.py @@ -16,7 +16,7 @@ sys.path.insert(0, str(folder)) from health_azure import submit_to_azure_if_needed # noqa: E402 -from health_azure.utils import create_argparser, parse_arguments # noqa: E402 +from health_azure.utils import WORKSPACE_CONFIG_JSON, check_config_json, create_argparser, parse_arguments # noqa: E402 from health_ml.utils.common_utils import DEFAULT_AML_UPLOAD_DIR # noqa: E402 from health_ml.utils.fixed_paths import repository_root_directory # noqa: E402 @@ -70,12 +70,18 @@ def logging_to_stdout() -> None: parser = create_argparser(config) parser_results = parse_arguments(parser, fail_on_unknown_args=True) config = RunPytestConfig(**parser_results.args) - submit_to_azure_if_needed( - compute_cluster_name=config.cluster, - submit_to_azureml=(config.cluster != ""), - wait_for_completion=True, - snapshot_root_directory=repository_root_directory(), - conda_environment_file=config.conda_env, - experiment_name=config.experiment, - ) + submit_to_azureml = config.cluster != "" + if submit_to_azureml: + # For runs on the github agents: Create a workspace config file from environment variables. + # For local runs, this will fall back to a config.json file in the current folder or at repository root + root_config_json = himl_root / WORKSPACE_CONFIG_JSON + with check_config_json(path=Path.cwd(), shared_config_json=root_config_json): + submit_to_azure_if_needed( + compute_cluster_name=config.cluster, + submit_to_azureml=submit_to_azureml, + wait_for_completion=True, + snapshot_root_directory=repository_root_directory(), + conda_environment_file=config.conda_env, + experiment_name=config.experiment, + ) run_pytest(folder_to_test=config.folder, pytest_mark=config.mark) From 53974e9249d11dcd45b8dabb1fedc4c37d74b02d Mon Sep 17 00:00:00 2001 From: Anton Schwaighofer Date: Tue, 29 Mar 2022 16:57:09 +0100 Subject: [PATCH 12/23] fixes --- hi-ml-azure/src/health_azure/utils.py | 2 +- hi-ml-azure/testazure/testazure/test_himl.py | 3 ++- hi-ml/run_pytest.py | 3 +-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/hi-ml-azure/src/health_azure/utils.py b/hi-ml-azure/src/health_azure/utils.py index be55d3511..bb8811c95 100644 --- a/hi-ml-azure/src/health_azure/utils.py +++ b/hi-ml-azure/src/health_azure/utils.py @@ -1981,7 +1981,7 @@ def workspace(self) -> Workspace: def check_config_json(script_folder: Path, shared_config_json: Path) -> Generator: """ Create a workspace config.json file exists in the folder where we expect a test script. This is either copied - from the location given in shared_config_json (this should be the case when executing a test on a dev machine), + from the location given in shared_config_json (this should be the case when executing a test on a dev machine), or created from environment variables (this should trigger in builds on the github agents). :param script_folder: This is the folder in which the config.json file should be created diff --git a/hi-ml-azure/testazure/testazure/test_himl.py b/hi-ml-azure/testazure/testazure/test_himl.py index 350084c58..6b4557d85 100644 --- a/hi-ml-azure/testazure/testazure/test_himl.py +++ b/hi-ml-azure/testazure/testazure/test_himl.py @@ -34,7 +34,8 @@ from health_azure.utils import (ENVIRONMENT_VERSION, EXPERIMENT_RUN_SEPARATOR, WORKSPACE_CONFIG_JSON, check_config_json, get_most_recent_run, get_workspace, is_running_in_azure_ml) from testazure.test_data.make_tests import render_environment_yaml, render_test_script -from testazure.utils_testazure import DEFAULT_DATASTORE, change_working_directory, get_shared_config_json, repository_root +from testazure.utils_testazure import (DEFAULT_DATASTORE, change_working_directory, get_shared_config_json, + repository_root) INEXPENSIVE_TESTING_CLUSTER_NAME = "lite-testing-ds2" EXPECTED_QUEUED = "This command will be run in AzureML:" diff --git a/hi-ml/run_pytest.py b/hi-ml/run_pytest.py index 140ba4f5f..642fc1bc7 100644 --- a/hi-ml/run_pytest.py +++ b/hi-ml/run_pytest.py @@ -1,7 +1,6 @@ import logging import sys from pathlib import Path -from typing import Tuple import pytest import param @@ -75,7 +74,7 @@ def logging_to_stdout() -> None: # For runs on the github agents: Create a workspace config file from environment variables. # For local runs, this will fall back to a config.json file in the current folder or at repository root root_config_json = himl_root / WORKSPACE_CONFIG_JSON - with check_config_json(path=Path.cwd(), shared_config_json=root_config_json): + with check_config_json(script_folder=Path.cwd(), shared_config_json=root_config_json): submit_to_azure_if_needed( compute_cluster_name=config.cluster, submit_to_azureml=submit_to_azureml, From 44ef2acc36430406511e45f129d8c392df749435 Mon Sep 17 00:00:00 2001 From: Anton Schwaighofer Date: Tue, 29 Mar 2022 17:04:24 +0100 Subject: [PATCH 13/23] flake --- hi-ml-histopathology/testSSL/testSSL/utils.py | 8 +------- .../testhisto/testhisto/utils/utils_testhisto.py | 4 +--- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/hi-ml-histopathology/testSSL/testSSL/utils.py b/hi-ml-histopathology/testSSL/testSSL/utils.py index 9dcdcfb9b..7b82ee5d5 100644 --- a/hi-ml-histopathology/testSSL/testSSL/utils.py +++ b/hi-ml-histopathology/testSSL/testSSL/utils.py @@ -2,19 +2,13 @@ # Copyright (c) Microsoft Corporation. All rights reserved. # Licensed under the MIT License (MIT). See LICENSE in the repo root for license information. # ------------------------------------------------------------------------------------------ -import json -import logging -import os -import shutil -from contextlib import contextmanager from pathlib import Path -from typing import Generator, Optional +from typing import Optional import numpy as np import pydicom import SimpleITK as sitk from SSL.data.io_util import PhotometricInterpretation -from health_azure.utils import PathOrString ENV_RESOURCE_GROUP = "HIML_RESOURCE_GROUP" ENV_SUBSCRIPTION_ID = "HIML_SUBSCRIPTION_ID" diff --git a/hi-ml-histopathology/testhisto/testhisto/utils/utils_testhisto.py b/hi-ml-histopathology/testhisto/testhisto/utils/utils_testhisto.py index 1db732287..91a94bb44 100644 --- a/hi-ml-histopathology/testhisto/testhisto/utils/utils_testhisto.py +++ b/hi-ml-histopathology/testhisto/testhisto/utils/utils_testhisto.py @@ -4,15 +4,13 @@ # ------------------------------------------------------------------------------------------ import os from pathlib import Path -from typing import Any, Callable, Collection, Mapping, Optional, Sequence +from typing import Any, Callable, Collection, Mapping, Sequence import numpy as np import torch import torch.distributed from PIL import Image -from health_azure.utils import PathOrString - def assert_dicts_equal(d1: Mapping, d2: Mapping, exclude_keys: Collection[Any] = (), rtol: float = 1e-5, atol: float = 1e-8) -> None: From d58b87c59da543c07c7aee01ceead1ef76fc63d5 Mon Sep 17 00:00:00 2001 From: Anton Schwaighofer Date: Tue, 29 Mar 2022 17:48:24 +0100 Subject: [PATCH 14/23] amlignore --- .amlignore | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/.amlignore b/.amlignore index 4f7a353b6..98c4e3948 100644 --- a/.amlignore +++ b/.amlignore @@ -3,6 +3,8 @@ /azure-pipelines /docs /.idea +outputs/ +__pycache__/ .pytest_cache .mypy_cache logs @@ -14,3 +16,11 @@ temp_environment-* .config .vscode node_modules + +# Temp folders created from SSL tests +cifar-10-python.tar.gz +cifar-10-batches-py/ +None/ +hi-ml-histopathology/testSSL/test_outputs +test_outputs/ +lightning_logs/ From eeaaa75427f73d101f16bc0ce2bd5bcee9e05638 Mon Sep 17 00:00:00 2001 From: Anton Schwaighofer Date: Tue, 29 Mar 2022 17:48:30 +0100 Subject: [PATCH 15/23] fast trial --- .github/workflows/build-test-pr.yml | 36 +++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/.github/workflows/build-test-pr.yml b/.github/workflows/build-test-pr.yml index 2943f132a..8b8a69c84 100644 --- a/.github/workflows/build-test-pr.yml +++ b/.github/workflows/build-test-pr.yml @@ -24,6 +24,42 @@ env: HIML_COV_XML_ARTIFACT_SUFFIX: '-code_coverage_xml' jobs: + + run_gpu: + runs-on: ubuntu-18.04 + strategy: + matrix: + folder: [ hi-ml-histopathology ] + packageName: [ '*.whl' ] + steps: + - uses: actions/checkout@v2 + with: + lfs: true + + - name: Set up Python ${{ env.pythonVersion }} + uses: actions/setup-python@v2 + with: + python-version: ${{ env.pythonVersion }} + + - name: PIP upgrade + run: | + cd hi-ml-azure + make pip_upgrade + + - name: Install dependencies + run: | + cd hi-ml-azure + pip install ${{ steps.download_himlazure.outputs.package_filename }} + cd ../hi-ml + pip install ${{ steps.download_himl.outputs.package_filename }} + + - name: Run GPU tests + run: | + branch_prefix="refs/heads/" + full_branch_name=$GITHUB_REF + branch_name_without_prefix=${full_branch_name#$branch_prefix} + python hi-ml/run_pytest.py --mark=gpu --cluster=pr-gpu --conda_env=${{ matrix.folder }}/environment.yml --folder=${{ matrix.folder }} --experiment="$branch_name_without_prefix" + flake8: runs-on: ubuntu-18.04 steps: From 39a5f46dbe9de90ccb136287a8e2597431a4c613 Mon Sep 17 00:00:00 2001 From: Anton Schwaighofer Date: Tue, 29 Mar 2022 17:49:03 +0100 Subject: [PATCH 16/23] undo colors --- .vscode/settings.json | 5 ----- 1 file changed, 5 deletions(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index 1df60e0ba..cc760b8b1 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -73,9 +73,4 @@ "terminal.integrated.env.linux": { "PYTHONPATH":"${workspaceFolder}/hi-ml/src:${workspaceFolder}/hi-ml-azure/src:${workspaceFolder}/hi-ml-histopathology/src" }, - "workbench.colorCustomizations": { - "activityBar.background": "#077203", - "titleBar.activeBackground": "#077203", - "titleBar.activeForeground": "#FBFAF6" - } } \ No newline at end of file From ab7899bd4794273f6e340fb9ecf6971ecf0b5a0d Mon Sep 17 00:00:00 2001 From: Anton Schwaighofer Date: Tue, 29 Mar 2022 18:20:04 +0100 Subject: [PATCH 17/23] fix gpu test problem --- hi-ml/run_pytest.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/hi-ml/run_pytest.py b/hi-ml/run_pytest.py index 642fc1bc7..02eb91bbc 100644 --- a/hi-ml/run_pytest.py +++ b/hi-ml/run_pytest.py @@ -15,7 +15,13 @@ sys.path.insert(0, str(folder)) from health_azure import submit_to_azure_if_needed # noqa: E402 -from health_azure.utils import WORKSPACE_CONFIG_JSON, check_config_json, create_argparser, parse_arguments # noqa: E402 +from health_azure.utils import ( # noqa: E402 + WORKSPACE_CONFIG_JSON, + check_config_json, + create_argparser, + is_running_in_azure_ml, + parse_arguments, +) from health_ml.utils.common_utils import DEFAULT_AML_UPLOAD_DIR # noqa: E402 from health_ml.utils.fixed_paths import repository_root_directory # noqa: E402 @@ -70,7 +76,7 @@ def logging_to_stdout() -> None: parser_results = parse_arguments(parser, fail_on_unknown_args=True) config = RunPytestConfig(**parser_results.args) submit_to_azureml = config.cluster != "" - if submit_to_azureml: + if submit_to_azureml and not is_running_in_azure_ml(): # For runs on the github agents: Create a workspace config file from environment variables. # For local runs, this will fall back to a config.json file in the current folder or at repository root root_config_json = himl_root / WORKSPACE_CONFIG_JSON From 8a1f635581b3186357bca5f40e1a805fc54d4968 Mon Sep 17 00:00:00 2001 From: Anton Schwaighofer Date: Tue, 29 Mar 2022 21:24:03 +0100 Subject: [PATCH 18/23] undo debug change --- .github/workflows/build-test-pr.yml | 35 ----------------------------- 1 file changed, 35 deletions(-) diff --git a/.github/workflows/build-test-pr.yml b/.github/workflows/build-test-pr.yml index 8b8a69c84..536ec52c9 100644 --- a/.github/workflows/build-test-pr.yml +++ b/.github/workflows/build-test-pr.yml @@ -25,41 +25,6 @@ env: jobs: - run_gpu: - runs-on: ubuntu-18.04 - strategy: - matrix: - folder: [ hi-ml-histopathology ] - packageName: [ '*.whl' ] - steps: - - uses: actions/checkout@v2 - with: - lfs: true - - - name: Set up Python ${{ env.pythonVersion }} - uses: actions/setup-python@v2 - with: - python-version: ${{ env.pythonVersion }} - - - name: PIP upgrade - run: | - cd hi-ml-azure - make pip_upgrade - - - name: Install dependencies - run: | - cd hi-ml-azure - pip install ${{ steps.download_himlazure.outputs.package_filename }} - cd ../hi-ml - pip install ${{ steps.download_himl.outputs.package_filename }} - - - name: Run GPU tests - run: | - branch_prefix="refs/heads/" - full_branch_name=$GITHUB_REF - branch_name_without_prefix=${full_branch_name#$branch_prefix} - python hi-ml/run_pytest.py --mark=gpu --cluster=pr-gpu --conda_env=${{ matrix.folder }}/environment.yml --folder=${{ matrix.folder }} --experiment="$branch_name_without_prefix" - flake8: runs-on: ubuntu-18.04 steps: From 087c655284f3465cb08407c27850ca6f573d5318 Mon Sep 17 00:00:00 2001 From: Anton Schwaighofer Date: Thu, 31 Mar 2022 06:24:12 +0100 Subject: [PATCH 19/23] improve docu --- hi-ml-histopathology/README.md | 32 ++++++++++++++++++++++++++++++++ hi-ml-histopathology/pytest.ini | 2 +- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/hi-ml-histopathology/README.md b/hi-ml-histopathology/README.md index c09540ac2..e7ac3e294 100644 --- a/hi-ml-histopathology/README.md +++ b/hi-ml-histopathology/README.md @@ -52,3 +52,35 @@ make call_pytest Inside of VSCode, all tests in the repository should be picked up automatically. You can exclude the tests for the `hi-ml` and `hi-ml-azure` packages by modifying `python.testing.pytestArgs` in the VSCode `.vscode/settings.json` file. + +## Tests that require a GPU + +The test pipeline for the histopathology folder contains a run of `pytest` on a machine with 2 GPUs. Only tests that are +marked with the `pytest` mark `gpu` are executed on that GPU machine. Note that all tests that bear the `gpu` mark will +_also_ be executed when running on a CPU machine. You need to manually add a `skipif` flag for tests that are meant to +exclusively run on GPU machines. This also helps to ensure that the test suite can pass when executed outside of the +build agents. + +* Tests that run only on a CPU machine: Provide no `pytest` marks + +```python +def test_my_code() -> None: + pass +``` + +* Tests that run on both on a CPU and on a GPU machine: Add `@pytest.mark.gpu` + +```python +@pytest.mark.gpu +def test_my_code() -> None: + pass +``` + +* Tests that run only on a GPU machine: + +```python +@pytest.mark.skipif(torch.cuda.device_count() < 1, reason="This test requires a GPU") +@pytest.mark.gpu +def test_my_code() -> None: + pass +``` diff --git a/hi-ml-histopathology/pytest.ini b/hi-ml-histopathology/pytest.ini index 3a848af86..5f801ba30 100644 --- a/hi-ml-histopathology/pytest.ini +++ b/hi-ml-histopathology/pytest.ini @@ -6,4 +6,4 @@ log_cli_level = DEBUG addopts = --strict-markers markers = fast: Tests that should run very fast, and can act as smoke tests to see if something goes terribly wrong. - gpu: Tests that require at least 1 GPU to be present + gpu: Tests that should be executed both on a normal machine and on a machine with 2 GPUs. From 6e80e1b88b503fd0119b7a3f7f833acb9ede1c3b Mon Sep 17 00:00:00 2001 From: Anton Schwaighofer Date: Fri, 1 Apr 2022 11:15:56 +0100 Subject: [PATCH 20/23] PR comments --- hi-ml-azure/src/health_azure/utils.py | 6 +++--- hi-ml/run_pytest.py | 11 +---------- 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/hi-ml-azure/src/health_azure/utils.py b/hi-ml-azure/src/health_azure/utils.py index bb8811c95..654c4fbc0 100644 --- a/hi-ml-azure/src/health_azure/utils.py +++ b/hi-ml-azure/src/health_azure/utils.py @@ -2004,9 +2004,9 @@ def check_config_json(script_folder: Path, shared_config_json: Path) -> Generato if subscription_id and resource_group and workspace_name: with open(str(target_config_json), 'w', encoding="utf-8") as file: config = { - "subscription_id": os.getenv(ENV_SUBSCRIPTION_ID, ""), - "resource_group": os.getenv(ENV_RESOURCE_GROUP, ""), - "workspace_name": os.getenv(ENV_WORKSPACE_NAME, "") + "subscription_id": subscription_id, + "resource_group": resource_group, + "workspace_name": workspace_name } json.dump(config, file) else: diff --git a/hi-ml/run_pytest.py b/hi-ml/run_pytest.py index 02eb91bbc..cae14c822 100644 --- a/hi-ml/run_pytest.py +++ b/hi-ml/run_pytest.py @@ -22,7 +22,7 @@ is_running_in_azure_ml, parse_arguments, ) -from health_ml.utils.common_utils import DEFAULT_AML_UPLOAD_DIR # noqa: E402 +from health_ml.utils.common_utils import DEFAULT_AML_UPLOAD_DIR, logging_to_stdout # noqa: E402 from health_ml.utils.fixed_paths import repository_root_directory # noqa: E402 PYTEST_RESULTS_FILE = "pytest_results.xml" @@ -58,15 +58,6 @@ def run_pytest(folder_to_test: str, pytest_mark: str) -> None: raise ValueError(f"PyTest failed with exit code: {status_code}") -def logging_to_stdout() -> None: - """Redirects output from the logging module to stdout.""" - root = logging.getLogger() - root.setLevel(logging.DEBUG) - handler = logging.StreamHandler(sys.stdout) - handler.setLevel(logging.INFO) - root.addHandler(handler) - - if __name__ == "__main__": logging_to_stdout() From 63ea6664e8a53689a5737db3207307b7e698a9eb Mon Sep 17 00:00:00 2001 From: Anton Schwaighofer Date: Fri, 1 Apr 2022 14:23:20 +0100 Subject: [PATCH 21/23] improve runner --- hi-ml-azure/src/health_azure/utils.py | 18 +++++- .../testazure/testazure/test_azure_util.py | 56 ++++++++++++++++--- hi-ml/run_pytest.py | 23 ++++++-- 3 files changed, 82 insertions(+), 15 deletions(-) diff --git a/hi-ml-azure/src/health_azure/utils.py b/hi-ml-azure/src/health_azure/utils.py index 654c4fbc0..879d2dca3 100644 --- a/hi-ml-azure/src/health_azure/utils.py +++ b/hi-ml-azure/src/health_azure/utils.py @@ -176,15 +176,29 @@ def set_fields_and_validate(config: param.Parameterized, fields_to_set: Dict[str config.validate() -def create_argparser(config: param.Parameterized) -> ArgumentParser: +def create_argparser( + config: param.Parameterized, + usage: Optional[str] = None, + description: Optional[str] = None, + epilog: Optional[str] = None +) -> ArgumentParser: """ Creates an ArgumentParser with all fields of the given config that are overridable. :param config: The config whose parameters should be used to populate the argument parser + :param usage: Brief information about correct usage that is printed if the script started with "--help". If not + provided, this is auto-generated from the complete set of arguments. + :param description: A description of the program that is printed if the script is started with "--help" + :param epilog: A text that is printed after the argument details if the script is started with "--help" :return: ArgumentParser """ assert isinstance(config, param.Parameterized) - parser = ArgumentParser(formatter_class=ArgumentDefaultsHelpFormatter) + parser = ArgumentParser( + formatter_class=ArgumentDefaultsHelpFormatter, + usage=usage, + description=description, + epilog=epilog + ) _add_overrideable_config_args_to_parser(config, parser) return parser diff --git a/hi-ml-azure/testazure/testazure/test_azure_util.py b/hi-ml-azure/testazure/testazure/test_azure_util.py index 24008fd75..4a68a162b 100644 --- a/hi-ml-azure/testazure/testazure/test_azure_util.py +++ b/hi-ml-azure/testazure/testazure/test_azure_util.py @@ -22,7 +22,7 @@ import numpy as np import param import pytest -from _pytest.capture import CaptureFixture +from _pytest.capture import SysCapture from _pytest.logging import LogCaptureFixture from azureml.core import Experiment, Run, ScriptRunConfig, Workspace from azureml.core.authentication import ServicePrincipalAuthentication @@ -32,7 +32,7 @@ import health_azure.utils as util from health_azure.himl import AML_IGNORE_FILE, append_to_amlignore -from health_azure.utils import PackageDependency +from health_azure.utils import PackageDependency, create_argparser from testazure.test_himl import RunTarget, render_and_run_test_script from testazure.utils_testazure import (DEFAULT_IGNORE_FOLDERS, DEFAULT_WORKSPACE, MockRun, change_working_directory, repository_root) @@ -579,7 +579,7 @@ def test_merge_conda_with_pip_requirements(random_folder: Path) -> None: @pytest.mark.fast -def test_merge_conda_failure_cases(random_folder: Path, caplog: CaptureFixture) -> None: +def test_merge_conda_failure_cases(random_folder: Path, caplog: LogCaptureFixture) -> None: """Tests various failure cases of merging, such as empty conda files, no specified channels etc""" merged_file = random_folder / "merged.yml" env1 = _generate_conda_env_str(channels=["defaults", "pytorch"], @@ -1004,8 +1004,8 @@ def _mock_env_get(workspace: Workspace, name: str = "", version: Optional[str] = def test_set_environment_variables_for_multi_node( - caplog: CaptureFixture, - capsys: CaptureFixture, + caplog: LogCaptureFixture, + capsys: SysCapture, ) -> None: with caplog.at_level(logging.INFO): # type: ignore util.set_environment_variables_for_multi_node() @@ -1022,7 +1022,7 @@ def test_set_environment_variables_for_multi_node( }, clear=True): util.set_environment_variables_for_multi_node() - out, _ = capsys.readouterr() + out = capsys.readouterr().out assert "Distributed training: MASTER_ADDR = here, MASTER_PORT = there, NODE_RANK = everywhere" in out with mock.patch.dict( @@ -1034,7 +1034,7 @@ def test_set_environment_variables_for_multi_node( }, clear=True): util.set_environment_variables_for_multi_node() - out, _ = capsys.readouterr() + out = capsys.readouterr().out assert "Distributed training: MASTER_ADDR = here, MASTER_PORT = 6105, NODE_RANK = everywhere" in out @@ -1883,6 +1883,48 @@ def test_parsing_bools(parameterized_config_and_parser: Tuple[ParamClass, Argume expected_value) +def test_argparse_usage(capsys: SysCapture) -> None: + """Test if the auto-generated argument parser prints out defaults and usage information. + """ + class SimpleClass(param.Parameterized): + name: str = param.String(default="name_default", doc="Name description") + config = SimpleClass() + parser = create_argparser(config, usage="my_usage", description="my_description", epilog="my_epilog") + arguments = ["", "--help"] + with pytest.raises(SystemExit): + with patch.object(sys, "argv", arguments): + parser.parse_args() + stdout: str = capsys.readouterr().out # type: ignore + assert "Name description" in stdout + assert "default: " in stdout + assert "optional arguments:" in stdout + assert "--name NAME" in stdout + assert "usage: my_usage" in stdout + assert "my_description" in stdout + assert "my_epilog" in stdout + + +def test_argparse_usage_empty(capsys: SysCapture) -> None: + """Test if the auto-generated argument parser prints out defaults and auto-generated usage information. + """ + class SimpleClass(param.Parameterized): + name: str = param.String(default="name_default", doc="Name description") + config = SimpleClass() + parser = create_argparser(config) + arguments = ["", "--help"] + with pytest.raises(SystemExit): + with patch.object(sys, "argv", arguments): + parser.parse_args() + stdout: str = capsys.readouterr().out # type: ignore + assert "usage: " in stdout + # Check if the auto-generated usage text is present + assert "[-h] [--name NAME]" in stdout + assert "optional arguments:" in stdout + assert "--name NAME" in stdout + assert "Name description" in stdout + assert "default: " in stdout + + @pytest.mark.fast def test_apply_overrides(parameterized_config_and_parser: Tuple[ParamClass, ArgumentParser]) -> None: """ diff --git a/hi-ml/run_pytest.py b/hi-ml/run_pytest.py index cae14c822..d5848c8f6 100644 --- a/hi-ml/run_pytest.py +++ b/hi-ml/run_pytest.py @@ -30,10 +30,18 @@ class RunPytestConfig(param.Parameterized): mark: str = param.String(default="", doc="The value to pass to pytest for the -m (mark) argument.") - folder: str = param.String(default="", doc="The folder of tests that should be run.") + folder: str = param.String( + default="", + doc="The file or folder of tests that should be run. This value is used as the first argument to start " + "pytest, so it can also be a specific test like 'my_test.py::any_test'", + ) cluster: str = param.String(default="", doc="The name of the AzureML compute cluster where the script should run.") - conda_env: str = param.String(default="", doc="The path to the Conda environment file that should be used.") - experiment: str = param.String(default="", doc="The name of the AzureML experiment where the run should live.") + conda_env: str = param.String( + default="", doc="The path to the Conda environment file that should be used when starting pytest in AzureML." + ) + experiment: str = param.String( + default="run_pytest", doc="The name of the AzureML experiment where the run should start." + ) def run_pytest(folder_to_test: str, pytest_mark: str) -> None: @@ -59,13 +67,16 @@ def run_pytest(folder_to_test: str, pytest_mark: str) -> None: if __name__ == "__main__": - logging_to_stdout() - config = RunPytestConfig() - parser = create_argparser(config) + parser = create_argparser( + config, + description="Invoke pytest either locally or inside of an AzureML run. The value of the '--folder' option is " + "becoming the first argument to pytest.To run on AzureML, provide the '--cluster' option.", + ) parser_results = parse_arguments(parser, fail_on_unknown_args=True) config = RunPytestConfig(**parser_results.args) + logging_to_stdout() submit_to_azureml = config.cluster != "" if submit_to_azureml and not is_running_in_azure_ml(): # For runs on the github agents: Create a workspace config file from environment variables. From dbb588f0aaa1aff8b47ef9d7a37d9f634596e687 Mon Sep 17 00:00:00 2001 From: Anton Schwaighofer Date: Fri, 1 Apr 2022 14:32:45 +0100 Subject: [PATCH 22/23] mypy --- hi-ml-azure/src/health_azure/utils.py | 2 +- hi-ml-azure/testazure/testazure/test_azure_util.py | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/hi-ml-azure/src/health_azure/utils.py b/hi-ml-azure/src/health_azure/utils.py index 879d2dca3..03537a87a 100644 --- a/hi-ml-azure/src/health_azure/utils.py +++ b/hi-ml-azure/src/health_azure/utils.py @@ -186,7 +186,7 @@ def create_argparser( Creates an ArgumentParser with all fields of the given config that are overridable. :param config: The config whose parameters should be used to populate the argument parser - :param usage: Brief information about correct usage that is printed if the script started with "--help". If not + :param usage: Brief information about correct usage that is printed if the script started with "--help". If not provided, this is auto-generated from the complete set of arguments. :param description: A description of the program that is printed if the script is started with "--help" :param epilog: A text that is printed after the argument details if the script is started with "--help" diff --git a/hi-ml-azure/testazure/testazure/test_azure_util.py b/hi-ml-azure/testazure/testazure/test_azure_util.py index 4a68a162b..514ef2b4c 100644 --- a/hi-ml-azure/testazure/testazure/test_azure_util.py +++ b/hi-ml-azure/testazure/testazure/test_azure_util.py @@ -22,7 +22,7 @@ import numpy as np import param import pytest -from _pytest.capture import SysCapture +from _pytest.capture import CaptureFixture from _pytest.logging import LogCaptureFixture from azureml.core import Experiment, Run, ScriptRunConfig, Workspace from azureml.core.authentication import ServicePrincipalAuthentication @@ -1005,7 +1005,7 @@ def _mock_env_get(workspace: Workspace, name: str = "", version: Optional[str] = def test_set_environment_variables_for_multi_node( caplog: LogCaptureFixture, - capsys: SysCapture, + capsys: CaptureFixture, ) -> None: with caplog.at_level(logging.INFO): # type: ignore util.set_environment_variables_for_multi_node() @@ -1883,7 +1883,7 @@ def test_parsing_bools(parameterized_config_and_parser: Tuple[ParamClass, Argume expected_value) -def test_argparse_usage(capsys: SysCapture) -> None: +def test_argparse_usage(capsys: CaptureFixture) -> None: """Test if the auto-generated argument parser prints out defaults and usage information. """ class SimpleClass(param.Parameterized): @@ -1904,7 +1904,7 @@ class SimpleClass(param.Parameterized): assert "my_epilog" in stdout -def test_argparse_usage_empty(capsys: SysCapture) -> None: +def test_argparse_usage_empty(capsys: CaptureFixture) -> None: """Test if the auto-generated argument parser prints out defaults and auto-generated usage information. """ class SimpleClass(param.Parameterized): From 33194e46508a603d46f8091a9013c9cb3988c67e Mon Sep 17 00:00:00 2001 From: Anton Schwaighofer Date: Mon, 4 Apr 2022 10:04:59 +0200 Subject: [PATCH 23/23] flake --- hi-ml-azure/testazure/testazure/test_himl.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/hi-ml-azure/testazure/testazure/test_himl.py b/hi-ml-azure/testazure/testazure/test_himl.py index 8855dcfaf..1d8feb951 100644 --- a/hi-ml-azure/testazure/testazure/test_himl.py +++ b/hi-ml-azure/testazure/testazure/test_himl.py @@ -31,9 +31,9 @@ import health_azure.himl as himl from health_azure.datasets import ( - DatasetConfig, - _input_dataset_key, - _output_dataset_key, + DatasetConfig, + _input_dataset_key, + _output_dataset_key, get_datastore ) from health_azure.utils import (