Skip to content

[WIP] Fix code based on review comments#657

Closed
sadamov with Copilot wants to merge 9 commits into
mainfrom
copilot/fix-comments-in-review-thread
Closed

[WIP] Fix code based on review comments#657
sadamov with Copilot wants to merge 9 commits into
mainfrom
copilot/fix-comments-in-review-thread

Conversation

Copilot AI commented Jun 9, 2026

Copy link
Copy Markdown
  • Inspect PR test: register slow pytest marker and apply to training tests #651 review thread comments and identify required fixes
  • Run baseline lint/test commands to capture pre-change state
  • Add minimal pytest marker registration required by review comments
  • Add focused test coverage for marker registration behavior
  • Run targeted tests to validate the fix
  • Run security checks (secret scan and CodeQL) and finalize

sadamov and others added 7 commits June 7, 2026 16:10
Register a `slow` marker in `pyproject.toml` and a matching
`--run-slow` CLI flag in `tests/conftest.py`. Tests carrying
`@pytest.mark.slow` are skipped by default and can be opted into
via `pytest --run-slow`.

Mark `test_training` (parametrised over all datastores) and
`test_training_output_std` as slow because they run a real
`trainer.fit` loop on the MDP / npyfilesmeps datastores and take
minutes per parametrisation. The fast unit tests in the suite
(`test_all_gather_cat_*`, datastore tests against the dummy fixture,
etc.) continue to run on every `pytest` invocation.

Motivated by #635 which already needed this mechanism for the ERA5
boundary integration test. Landing the infra separately so it's
reusable across the suite (e.g. the `test_state_only_datastore_*`
training test on #231 and future trainer.fit-based regressions).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Default pytest skips tests marked @pytest.mark.slow. CI should always
run the full suite (including trainer.fit-based regressions) so PRs
targeting main can't ship behaviour breakage that local-default
pytest would miss.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per @observingClouds's review on #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>
Co-authored-by: Hauke Schulz <43613877+observingClouds@users.noreply.github.com>
Removed pytest markers configuration from pyproject.toml.
@sadamov sadamov closed this Jun 9, 2026
Copilot AI requested a review from sadamov June 9, 2026 08:25
Copilot stopped work on behalf of sadamov due to an error June 9, 2026 08:25
@sadamov sadamov deleted the copilot/fix-comments-in-review-thread branch June 9, 2026 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants