diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 8dbd2ed2f..b37d5ee0a 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -53,8 +53,11 @@ on: description: "HubAI API key" required: true pull_request: + types: [opened, synchronize, reopened, ready_for_review] paths: - ".python-version" + - ".gitignore" + - "**/*.md" - "pyproject.toml" - "uv.lock" - "requirements*.txt" @@ -63,7 +66,6 @@ on: - "luxonis_train/**" - "tests/**" - ".github/workflows/ci.yaml" - - "!**/*.md" concurrency: # External callers can provide an isolated concurrency group. @@ -74,6 +76,42 @@ permissions: contents: read jobs: + # Selects the GPU suites. A change of the documentation only runs none + # (see scripts/only_docstrings_changed.py), and a draft skips the heavy one. + plan: + runs-on: ubuntu-latest + outputs: + gpu_suites: ${{ steps.plan.outputs.gpu_suites }} + code_changed: ${{ steps.plan.outputs.code_changed }} + steps: + - name: Checkout + if: ${{ github.event_name == 'pull_request' && !inputs.train_ref }} + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 + with: + ref: ${{ github.event.pull_request.head.sha }} + fetch-depth: 0 + persist-credentials: false + + - name: Select the test suites + id: plan + env: + PULL_REQUEST: ${{ github.event_name == 'pull_request' && !inputs.train_ref }} + DRAFT: ${{ github.event.pull_request.draft == true }} + BASE_SHA: ${{ github.event.pull_request.base.sha }} + run: | + code_changed=true + gpu_suites='["predefined_light", "predefined_heavy", "combinations", "misc"]' + if [[ "$PULL_REQUEST" == true ]]; then + if python3 scripts/only_docstrings_changed.py "$BASE_SHA"; then + code_changed=false + gpu_suites='[]' + elif [[ "$DRAFT" == true ]]; then + gpu_suites='["predefined_light", "combinations", "misc"]' + fi + fi + echo "code_changed=$code_changed" >> "$GITHUB_OUTPUT" + echo "gpu_suites=$gpu_suites" >> "$GITHUB_OUTPUT" + pre-commit: runs-on: ubuntu-latest steps: @@ -286,16 +324,18 @@ jobs: tests: needs: + - plan - config-test - doctests + # The suites come from `pytest_collection_modifyitems` in tests/conftest.py. strategy: matrix: - suite: [unit, predefined_light, predefined_heavy, combinations, misc] + suite: [cpu] - runs-on: ${{ matrix.suite == 'unit' && 'ubuntu-latest' || 'ubuntu-t4-4core' }} + runs-on: ubuntu-latest - steps: + steps: &test-steps - name: Checkout uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 with: @@ -363,10 +403,32 @@ jobs: if-no-files-found: ignore overwrite: true + # Waits for the `cpu` suite, so that its failures cost no GPU time. + gpu-tests: + # GitHub appends the suite. A skipped job then shows "tests". + name: tests + needs: + - plan + - tests + if: ${{ needs.plan.outputs.gpu_suites != '[]' }} + + strategy: + matrix: + suite: ${{ fromJSON(needs.plan.outputs.gpu_suites) }} + + runs-on: ubuntu-t4-4core + + steps: *test-steps + # Coverage is uploaded once, after all the suites have finished. Uploading # each suite on its own makes Codecov report the partial runs as the total. + # For the same reason, a change of the documentation only uploads nothing. coverage: - needs: tests + needs: + - plan + - tests + - gpu-tests + if: ${{ needs.plan.outputs.code_changed == 'true' }} runs-on: ubuntu-latest permissions: diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b08227533..9c544198c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -123,6 +123,11 @@ This command will run all tests and generate HTML coverage report. > [!TIP] > You can choose to run only the unit-tests or only the integration tests by adding `-m unit` or `-m integration` to the `pytest` command. +> [!IMPORTANT] +> Mark an integration test with `@pytest.mark.gpu` when it trains, tunes or quantizes a model. +> CI runs these tests on the GPU runner and all the other tests on a CPU runner. +> A test without the marker still passes on the CPU, but it no longer covers the GPU code of the training, such as mixed precision. + > [!IMPORTANT] > If a new feature is added, a new test should be added to cover it. > The minimum overall test coverage for a PR to be merged is 90%. @@ -136,6 +141,25 @@ Our GitHub Actions workflow is run when a new PR is opened. 1. Next, the [type checking](#type-checking) is run. 1. If all previous checks pass, the [tests](#tests) are run. +The tests run in five suites, one job each. +The GPU suites start only after the `cpu` suite passes, so that a failure there costs no GPU time: + +| Suite | Tests | Runner | +| -------------------------------------- | ------------------------------------------------------------------ | ----------------- | +| `cpu` | The unit tests, and the integration tests without the `gpu` marker | `ubuntu-latest` | +| `misc` | The integration tests with the `gpu` marker | `ubuntu-t4-4core` | +| `predefined_light`, `predefined_heavy` | `test_predefined_models.py`, the light and the heavy variants | `ubuntu-t4-4core` | +| `combinations` | `test_combinations.py` | `ubuntu-t4-4core` | + +CI does not run `test_overfit_convergence.py`, which has its own `overfit_convergence` marker. + +Two rules keep the GPU suites from running when they cannot find anything new: + +- A pull request that changes only docstrings, comments, Markdown files or the settings of the linters and of the API docs runs the `cpu` suite only. + `scripts/only_docstrings_changed.py` decides this. +- A draft pull request skips `predefined_heavy`. + The suite runs when you mark the pull request as ready for review. + > [!TIP] > Review the GitHub Actions output if your PR fails. diff --git a/scripts/only_docstrings_changed.py b/scripts/only_docstrings_changed.py new file mode 100644 index 000000000..1dc502e3e --- /dev/null +++ b/scripts/only_docstrings_changed.py @@ -0,0 +1,103 @@ +"""Tell whether a branch changes only its documentation. + +Exit with 0 when each changed file is a Markdown file, ``.gitignore``, +``.pre-commit-config.yaml``, ``pyproject.toml`` with changes in +``DOC_TABLES`` only, or a Python file whose syntax tree without +docstrings stays the same. Otherwise exit with 1 and name the file. + +Usage: ``python scripts/only_docstrings_changed.py `` + +""" + +import ast +import subprocess +import sys + +DOC_TABLES = ( + "tool.docformatter", + "tool.pydoctor", + "tool.pyright", + "tool.ruff", +) +IGNORED = {".gitignore", ".pre-commit-config.yaml"} + + +def git(*args: str) -> str: + """Run git and return its output.""" + return subprocess.run( + ["git", *args], # noqa: S607 + check=True, + capture_output=True, + text=True, + ).stdout + + +def is_bare_string(statement: ast.stmt) -> bool: + """Tell whether a statement is a string literal and nothing else.""" + return ( + isinstance(statement, ast.Expr) + and isinstance(statement.value, ast.Constant) + and isinstance(statement.value.value, str) + ) + + +def code_of(source: str) -> str: + """Dump the syntax tree of ``source`` without its docstrings. + + A string literal that stands as a statement has no effect, so the + docstrings of attributes go too. + + """ + tree = ast.parse(source) + for node in ast.walk(tree): + for field in ("body", "orelse", "finalbody"): + statements = getattr(node, field, None) + if isinstance(statements, list): + setattr( + node, + field, + [s for s in statements if not is_bare_string(s)], + ) + return ast.dump(tree) + + +def without_doc_tables(pyproject: str) -> list[str]: + """Drop the tables of ``DOC_TABLES`` from ``pyproject.toml``.""" + kept = [] + keep = True + for line in pyproject.splitlines(): + if line.startswith("["): + keep = not line.strip("[] ").startswith(DOC_TABLES) + if keep: + kept.append(line) + return kept + + +def main(base: str) -> int: + merge_base = git("merge-base", base, "HEAD").strip() + changes = git("diff", "--name-status", "--no-renames", merge_base, "HEAD") + for line in changes.splitlines(): + status, path = line.split("\t") + if path.endswith(".md") or path in IGNORED: + continue + if path == "pyproject.toml": + compare = without_doc_tables + elif path.endswith(".py"): + compare = code_of + else: + print(f"{path}: not a documentation file") + return 1 + if status != "M": + print(f"{path}: added or removed") + return 1 + before = git("show", f"{merge_base}:{path}") + after = git("show", f"HEAD:{path}") + if compare(before) != compare(after): + print(f"{path}: the code changed") + return 1 + print("Only the documentation changed.") + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1])) diff --git a/tests/conftest.py b/tests/conftest.py index fc0420d0d..af9e4e87d 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -427,27 +427,45 @@ def current_opset() -> int: def pytest_collection_modifyitems(items: list[Function]): + """Sort the tests into the CI suites, one marker for each suite. + + An integration test outside the model suites goes to ``misc`` when + it has the ``gpu`` marker, and to ``cpu`` otherwise. + + """ for item in items: path = str(item.fspath) if "/unittests/" in path: item.add_marker(pytest.mark.unit) + item.add_marker(pytest.mark.cpu) item.add_marker(pytest.mark.order(0)) elif "test_predefined_models.py" in path: item.add_marker(pytest.mark.predefined) item.add_marker(pytest.mark.order(2)) - elif "test_combinations.py" in path: + elif ( + "test_combinations.py" in path + and item.name != "test_combinations_archive" + ): item.add_marker(pytest.mark.combinations) item.add_marker(pytest.mark.order(3)) elif "test_overfit_convergence.py" in path: item.add_marker(pytest.mark.overfit_convergence) item.add_marker(pytest.mark.order(4)) else: - item.add_marker(pytest.mark.misc) + gpu = item.get_closest_marker("gpu") + item.add_marker(pytest.mark.misc if gpu else pytest.mark.cpu) item.add_marker(pytest.mark.order(1)) def pytest_configure(config: Config): config.addinivalue_line("markers", "unit: mark test as a unit test") + config.addinivalue_line( + "markers", + "gpu: mark test as a test that trains, tunes or quantizes a model", + ) + config.addinivalue_line( + "markers", "cpu: mark test as a test of the CPU suite" + ) config.addinivalue_line( "markers", "predefined: mark test as a predefined model test" ) @@ -463,7 +481,8 @@ def pytest_configure(config: Config): "markers", "combinations: mark test as a combinations test" ) config.addinivalue_line( - "markers", "misc: mark test as a miscellaneous test" + "markers", + "misc: mark test as a training test outside the model suites", ) config.addinivalue_line( "markers", diff --git a/tests/files/custom_archive_config.json b/tests/files/custom_archive_config.json index c82f005b8..78d83f7a4 100644 --- a/tests/files/custom_archive_config.json +++ b/tests/files/custom_archive_config.json @@ -31,7 +31,8 @@ ], "reverse_channels": null, "interleaved_to_planar": null, - "dai_type": "RGB888p" + "dai_type": "RGB888p", + "resize_mode": "LETTERBOX" } } ], diff --git a/tests/integration/test_callbacks.py b/tests/integration/test_callbacks.py index 32670a0e3..223a73c74 100644 --- a/tests/integration/test_callbacks.py +++ b/tests/integration/test_callbacks.py @@ -1,6 +1,7 @@ import json from pathlib import Path +import pytest import torch from luxonis_ml.data import LuxonisDataset from luxonis_ml.typing import Params @@ -8,6 +9,8 @@ from luxonis_train.config import Config from luxonis_train.core import LuxonisModel +pytestmark = pytest.mark.gpu + def test_callbacks(coco_dataset: LuxonisDataset, opts: Params, save_dir: Path): config_file = "luxonis_train/configs/segmentation_light_model.yaml" diff --git a/tests/integration/test_cli_commands.py b/tests/integration/test_cli_commands.py index cffb6ab44..58127a19c 100644 --- a/tests/integration/test_cli_commands.py +++ b/tests/integration/test_cli_commands.py @@ -34,6 +34,7 @@ def skip_if_no_aimet() -> None: pytest.skip("AIMET is not installed") +@pytest.mark.gpu def test_cli_command_success( coco_dataset: LuxonisDataset, tmp_path: Path, diff --git a/tests/integration/test_combinations.py b/tests/integration/test_combinations.py index 066dc4a00..8d5c91d2a 100644 --- a/tests/integration/test_combinations.py +++ b/tests/integration/test_combinations.py @@ -157,7 +157,26 @@ def test_combinations( with subtests.test("export"): model.export() - with subtests.test("archive"): - model.archive() - model.finalize_run() + + +def test_combinations_archive( + parking_lot_dataset: LuxonisDataset, + opts: Params, + request: pytest.FixtureRequest, +): + """Archive the model of `test_combinations` once. + + The archive reads the inputs and the outputs from the ONNX file and + the head configs from the heads. The backbone changes only the + output shapes, so one backbone is enough. `test_combinations` + exports every backbone. The archive does not depend on the weights, + so the test does not train. + + """ + assert request.node.get_closest_marker("cpu") is not None + opts |= { + "loader.params.dataset_name": parking_lot_dataset.identifier, + } + model = LuxonisModel(get_config("EfficientRep"), opts) + assert model.archive().exists() diff --git a/tests/integration/test_convert.py b/tests/integration/test_convert.py index 8981c3adc..7d4e61763 100644 --- a/tests/integration/test_convert.py +++ b/tests/integration/test_convert.py @@ -1,6 +1,7 @@ from pathlib import Path import pytest +from hubai_sdk.errors import HubApiError from luxonis_ml.data import LuxonisDataset from luxonis_ml.typing import Params @@ -62,6 +63,8 @@ def test_convert_with_blobconverter( # TODO: reintroduce Hailo conversion when modelconv is released and hub-ai is updated accordingly # TODO: reintroduce RVC3 conversion when remote-side conversion issue is resolved +# TODO: remove the xfail when the conversion on HubAI works again +@pytest.mark.xfail(raises=HubApiError) @pytest.mark.parametrize("platform", ["rvc2", "rvc4"]) def test_convert_with_hubai( coco_dataset: LuxonisDataset, opts: Params, tmp_path: Path, platform: str diff --git a/tests/integration/test_core.py b/tests/integration/test_core.py index ebdc88ff0..f5f4e6439 100644 --- a/tests/integration/test_core.py +++ b/tests/integration/test_core.py @@ -13,6 +13,7 @@ from luxonis_train.core import LuxonisModel +@pytest.mark.gpu @pytest.mark.skipif( sys.platform == "win32", reason="Tuning not supported on Windows" ) @@ -76,6 +77,7 @@ def test_weights_loading(cifar10_dataset: LuxonisDataset, opts: Params): ) +@pytest.mark.gpu def test_checkpoint( tmp_path: Path, opts: Params, coco_dataset: LuxonisDataset ): diff --git a/tests/integration/test_custom_model.py b/tests/integration/test_custom_model.py index 4e546319d..8b6ac67da 100644 --- a/tests/integration/test_custom_model.py +++ b/tests/integration/test_custom_model.py @@ -2,7 +2,9 @@ import tarfile from pathlib import Path +import pytest import torch +from luxonis_ml.nn_archive.config_building_blocks import PreprocessingBlock from luxonis_ml.typing import Params from pytest_subtests import SubTests from torch import Tensor, nn @@ -15,6 +17,8 @@ from luxonis_train.tasks import Tasks from luxonis_train.typing import Packet +pytestmark = pytest.mark.gpu + class CustomMultiInputLoader(BaseLoaderTorch): def __init__(self, **kwargs): @@ -140,6 +144,9 @@ def test_custom_model(opts: Params, tmp_path: Path, subtests: SubTests): correct_archive_config = json.loads( Path("tests/files/custom_archive_config.json").read_text() ) + if "resize_mode" not in PreprocessingBlock.model_fields: + for input_config in correct_archive_config["model"]["inputs"]: + input_config["preprocessing"].pop("resize_mode", None) with tarfile.open(archive_path) as tar: extracted_cfg = tar.extractfile("config.json") diff --git a/tests/integration/test_freezing.py b/tests/integration/test_freezing.py index 9affe1c95..3e5535d44 100644 --- a/tests/integration/test_freezing.py +++ b/tests/integration/test_freezing.py @@ -6,6 +6,8 @@ from luxonis_train.core import LuxonisModel +pytestmark = pytest.mark.gpu + # The frozen head owns a parameter group for the whole run, so both # series are logged from the first epoch. Only the head's group is diff --git a/tests/integration/test_loader.py b/tests/integration/test_loader.py index 3a2c4656e..66582928e 100644 --- a/tests/integration/test_loader.py +++ b/tests/integration/test_loader.py @@ -48,6 +48,7 @@ def test_splits( assert len(model.pytorch_loaders[key]) == exp_len +@pytest.mark.gpu def test_parsing(opts: Params): opts |= { "loader.params.dataset_name": "parsed_coco_test", @@ -62,6 +63,7 @@ def test_parsing(opts: Params): model.train() +@pytest.mark.gpu def test_dummy_loader(opts: Params): config_file = "luxonis_train/configs/complex_model.yaml" opts = opts | { diff --git a/tests/integration/test_mlflow_logging.py b/tests/integration/test_mlflow_logging.py index e1a888c90..aaa6363bb 100644 --- a/tests/integration/test_mlflow_logging.py +++ b/tests/integration/test_mlflow_logging.py @@ -81,6 +81,7 @@ def forward(self, x: Tensor) -> Tensor: return self.model(x) +@pytest.mark.gpu @pytest.mark.timeout(TIMEOUT) def test_mlflow_logging(xor_dataset: LuxonisDataset, subtests: SubTests): model = LuxonisModel( @@ -329,6 +330,7 @@ def get_config(trainer_overrides: Params | None = None) -> Params: ) +@pytest.mark.gpu @pytest.mark.timeout(TIMEOUT) def test_mlflow_logging_with_first_epoch_validation( xor_dataset: LuxonisDataset, subtests: SubTests diff --git a/tests/integration/test_overfit_batches.py b/tests/integration/test_overfit_batches.py index 2613d30d5..9df318c38 100644 --- a/tests/integration/test_overfit_batches.py +++ b/tests/integration/test_overfit_batches.py @@ -7,6 +7,8 @@ from luxonis_train.core import LuxonisModel +pytestmark = pytest.mark.gpu + @pytest.fixture(autouse=True) def reset_deterministic_state() -> Generator[None]: diff --git a/tests/integration/test_resume_training.py b/tests/integration/test_resume_training.py index a508ec97f..5ba7b8643 100644 --- a/tests/integration/test_resume_training.py +++ b/tests/integration/test_resume_training.py @@ -49,6 +49,7 @@ def _get_detection_light_train_opts( } +@pytest.mark.gpu def test_resume_training_with_ema_does_not_crash( parking_lot_dataset: LuxonisDataset, strict_loading_original_ckpt: Path, @@ -121,6 +122,7 @@ def test_training_with_unexpected_checkpoint_key_fails_when_strict( model.train(weights=strict_loading_modified_model_ckpt) +@pytest.mark.gpu def test_training_with_unexpected_checkpoint_key_succeeds_when_not_strict( parking_lot_dataset: LuxonisDataset, strict_loading_modified_model_ckpt: Path, diff --git a/tests/integration/test_scheduler.py b/tests/integration/test_scheduler.py index 4460738b5..941e303d9 100644 --- a/tests/integration/test_scheduler.py +++ b/tests/integration/test_scheduler.py @@ -6,6 +6,8 @@ from luxonis_train.core import LuxonisModel +pytestmark = pytest.mark.gpu + def sequential_scheduler() -> dict[str, Any]: return { diff --git a/tests/unittests/test_ci.py b/tests/unittests/test_ci.py new file mode 100644 index 000000000..dc206f2f0 --- /dev/null +++ b/tests/unittests/test_ci.py @@ -0,0 +1,14 @@ +from pathlib import Path + + +def test_documentation_changes_start_ci(): + workflow = Path(".github/workflows/ci.yaml").read_text() + pull_request = workflow.partition(" pull_request:\n")[2].partition( + "\n\n" + )[0] + documentation_paths = { + line.strip() + for line in pull_request.splitlines() + if ".md" in line or ".gitignore" in line + } + assert documentation_paths == {'- ".gitignore"', '- "**/*.md"'}