test: register slow pytest marker and apply to training tests#651
Conversation
Matches the CI invocation introduced in mllam#651 so this branch's eventual ERA5 boundary integration test (and any other @pytest.mark.slow on this branch) is exercised by CI, not just by local --run-slow invocations. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Matches the CI invocation introduced in mllam#651 so any @pytest.mark.slow test on this branch is exercised by CI, not just by local --run-slow invocations. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
observingClouds
left a comment
There was a problem hiding this comment.
This is a great idea @sadamov. I wonder if it is better to define the inverse of this, e.g. a --skip-slow flag that needs to be set. So by default the whole suite is run and only in the case when you are familiar with the testing, you add the --skip-slow flag to skip some tests.
Per @observingClouds's review on mllam#651: pytest already has built-in marker filtering, so we don't need the custom pytest_addoption + pytest_collection_modifyitems hook to support skip-by-default behaviour. Drop the custom flag entirely and rely on pytest's native -m marker selection: - `pytest` (no flags) runs the full suite, including slow tests (this restores the prior default behaviour, addresses observingClouds' safety concern). - `pytest -m "not slow"` skips slow tests for fast local iteration. - CI runs `pytest` unchanged - no --run-slow flag needed. Keep only the marker registration in pyproject.toml and the @pytest.mark.slow decorators on test_training and test_training_output_std. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
--run-slow pytest flag and mark training tests as slowslow pytest marker and apply to training tests
|
@observingClouds, I am glad you mentioned this. I just learned that there is a built-in |
Per the simplification in mllam#651, the custom --run-slow flag is being dropped in favour of relying on pytest's native -m marker selection. Remove the corresponding pytest_addoption + pytest_collection_modifyitems from conftest and the --run-slow flag from CI on this branch too, so this PR doesn't re-introduce conflicts when mllam#651 lands. The slow marker registration in pyproject.toml stays, ready for use on any future @pytest.mark.slow test on this branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per the simplification in mllam#651, the custom --run-slow flag is being dropped in favour of relying on pytest's native -m marker selection. Remove the corresponding pytest_addoption + pytest_collection_modifyitems from conftest and the --run-slow flag from CI on this branch too, so this PR doesn't re-introduce conflicts when mllam#651 lands. The @pytest.mark.slow on test_plot_prediction_with_era5_boundary stays; under the new mechanism that test runs by default (including in CI) and contributors can skip it locally with `pytest -m "not slow"`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CONTRIBUTING.md (this PR) now owns the general contributor workflow. Remove the duplicate sections from AGENTS.md (Issues, Pull Requests, Changelog) and add a pointer to CONTRIBUTING.md as the first thing agents should read. Kept and refined the AI-specific rules: - Search before creating issues/PRs (now consolidated to one section with the exact gh search commands). - Re-read the full thread before every action, reload after context gaps - the rule that matters most for agents. - Communication style (terse, no filler ban list). - AI attribution in commit trailers. Also updated the codebase reference to reflect the post-mllam#208 ForecasterModule / Forecaster / StepPredictor hierarchy, and added `pytest -m "not slow"` to the command list (per mllam#651). Net: 91 lines (down from 97), but the AI-specific content is denser. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
observingClouds
left a comment
There was a problem hiding this comment.
Just a few more small suggestions.
Add a `[tool.pytest.ini_options]` block to `pyproject.toml` that
registers a `slow` marker, and apply `@pytest.mark.slow` to
`test_training` and `test_training_output_std` so contributors can
skip the long training tests during local iteration:
pytest -m "not slow" # quick sweep, skips training tests
pytest -m slow # only the training tests
pytest # everything (CI default)
Closes mllam#651.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
a7e7d68 to
6dd6c07
Compare
`test_last_checkpoint_saved_without_validation` spins up a real `pl.Trainer` and runs a full training epoch on CPU; in practice this adds ~60s to a pytest run, same order as the `test_training` tests already marked `slow` in mllam#651. Tagging it here so it is grouped with the other long-running training tests and skipped by default during fast `pytest -m "not slow"` iteration. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The `slow` marker registration still described the markers as "deselected by default" (a leftover from the removed `--run-slow` flag). Slow tests actually run by default and are skipped with `-m "not slow"`, matching the behaviour agreed in mllam#651. Description-only change; no behaviour change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Describe your changes
Register a
slowpytest marker inpyproject.tomland apply@pytest.mark.slowtotest_training(parametrised over all datastores) andtest_training_output_stdbecause they run a realtrainer.fitloop and take minutes per parametrisation.This relies on pytest's built-in marker filtering rather than introducing a custom flag:
pytest(no flags) runs the full suite, slow tests included. CI uses this and continues to exercise everything.pytest -m "not slow"skips slow tests, for fast local iteration during contributor development.pytest -m slowruns only slow tests, e.g. when debugging a training regression.Motivation: I needed this in #635 for the ERA5 boundary integration test, then realised the mechanism is independently useful for the existing trainer.fit-based tests too. Landing the marker separately so #635 doesn't have to ship two things at once, and so #231's
test_state_only_datastore_training_setup_runs(and any future trainer.fit-based regression) can opt in via the same marker.Coordination with #615 (@jishanahmed-shaikh): orthogonal. The
slowmarker is about execution time, the unit/integration split is about scope. They coexist: an integration test can be fast (DummyDatastore) and a unit test can be slow (large numerical fixture).Earlier iteration of this PR introduced a custom
--run-slowCLI flag with skip-by-default behaviour. @observingClouds suggested the inverse (run-by-default with--skip-slow), and the cleanest resolution was to drop the custom flag entirely and rely on pytest's native-mselection. Pytest already supports exactly this pattern with zero custom code.No new dependencies. No CI changes.
Verified locally:
pytest tests/test_training.py --collect-only -q-> all 6 tests collectedpytest tests/test_training.py -m "not slow" --collect-only -q-> 2 collected, 4 deselectedIssue Link
Refs #635, refs #599
Type of change
Checklist before requesting a review
pullwith--rebaseoption if possible).Checklist for reviewers
Each PR comes with its own improvements and flaws. The reviewer should check the following:
Author checklist after completed review
reflecting type of change (add section where missing):
Checklist for assignee