Add Docker-based CI/CD for fast testing - #580
Closed
timlichtenberg wants to merge 58 commits into
Closed
timlichtenberg wants to merge 58 commits into
timlichtenberg wants to merge 58 commits into
Conversation
…thon versions and OS platforms
…d testing coverage
… in error message
The CI workflow uses 'coverage run -m pytest' to collect coverage data. Having --cov options in pytest addopts creates a conflict that prevents coverage measurement. Coverage reporting is still configured in [tool.coverage.report] section and will now work properly with the CI command.
Now that NumPy 2.0 fixes are merged to aragog main (PR #5), remove the temporary test branch reference from CI workflow. Related: FormingWorlds/aragog#5
Implements performance optimizations to reduce CI runtime: 1. Cache SOCRATES compiled binaries (~7-8 min savings) - Caches socrates/ directory with binaries - Key based on source file hashes for automatic invalidation - Restore-keys for partial cache hits 2. Cache AGNI Julia depot (~3-4 min savings) - Caches AGNI/ directory and ~/.julia/ packages - Key based on Julia source and manifest files - Restore-keys for partial cache hits Expected improvement: 10-12 minutes saved per CI run (from ~26 to ~14-16 minutes) These caches only rebuild when source files change, otherwise use cached binaries/packages from previous runs.
Previous attempt failed because: - Cache keys used hashFiles() on directories that didn't exist yet - Result: empty cache keys like 'socrates-bins-Linux-' - No cache was ever restored or saved properly New approach: - Use cache/restore before install-all to load previous build - Use cache/save after tests to save new build - Key based on run_id (unique) with restore-keys for prefix matching - Only save cache if restore missed (avoid duplicate saves) This allows second and subsequent runs to skip 10-12 minutes of compilation.
CRITICAL FIX: Cache keys now depend on source/dependency hashes instead of run ID. This ensures: 1. Cache is automatically INVALIDATED when source code changes 2. Cache is automatically INVALIDATED when dependencies change 3. Tests always use current SOCRATES and AGNI versions 4. No stale cached code is used if upstream repos change SOCRATES cache: - Key: hash of *.f90, *.F90, *.c files, and build_code script - Invalidates when any Fortran/C source changes AGNI cache: - Key: hash of Project.toml and Manifest.toml files - Invalidates when Julia dependencies change Benefits: ✓ 32% CI speedup (26m → 18m) when deps unchanged ✓ Automatic detection of upstream changes ✓ No stale cache issues ✓ Maintain testing integrity Note: Pre-existing linting warnings about env.total are unrelated to this change and do not affect workflow execution.
Skip disk cleanup when available space is >80%, saving ~2m30s per run Changes: - Add 'Check available disk space' step that calculates usage percentage - Modify 'Free Disk Space (Ubuntu)' condition to only run if usage >20% - Threshold can be tuned; 80% is conservative to prevent full disk Expected savings: 2m 30s per build (disk rarely critical) Impact: ~15% CI runtime reduction on test branches Risk: LOW - cleanup still triggers if disk space actually needed
Critical bug fix: hashFiles() was evaluating on non-existent directories Root cause: - Cache restore steps tried to hash 'socrates/**/*.f90' files - But socrates/ directory didn't exist yet (cloned later in install-all) - Result: Empty hash → cache key 'socrates-bins-Linux-' (missing hash) - Cache always missed → SOCRATES recompiled every run (+11-15 min) Solution: - Clone SOCRATES and AGNI repos BEFORE cache restore steps - Now hashFiles() can compute proper hashes - Cache keys like 'socrates-bins-Linux-abc123def456' work correctly - proteus install-all will use existing clones (no duplicate work) Expected impact: - Cache hits will now work properly - Saves 11-15 minutes when SOCRATES source unchanged - Saves 2-3 minutes when AGNI dependencies unchanged - Reduces run from 48m to ~17-19m when caches hit
…nd 69% coverage threshold Priority 1 improvements to testing_infrastructure.md: Changes: - Document PROTEUS Phase 1 completion (69.23% coverage achieved) - Add comprehensive Phase 2 ecosystem integration guide - Create 4-step quick start deployment for ecosystem modules - Add advanced hash-based caching strategy documentation - Update coverage threshold progression from 5% to 69% for PROTEUS - Change reusable workflow default threshold from 5% to 30% (realistic for new modules) - Add deployment checklist (~2 hours per module) - Include performance expectations and troubleshooting for caching Files modified: - docs/testing_infrastructure.md: +317 lines (comprehensive ecosystem rollout guide) - pyproject.toml: fail_under = 69 (enforces actual achieved coverage) - .github/workflows/proteus_test_quality_gate.yml: improved default threshold and guidance This enables ecosystem modules (CALLIOPE, JANUS, MORS, VULCAN, ZEPHYRUS) to deploy quality gates with clear configuration examples, realistic thresholds, and validated patterns from PROTEUS implementation.
- Add CALLIOPE as Phase 2 pilot reference implementation - Document coverage ratcheting mechanism (auto-threshold updates) - Establish ecosystem integration standards (Codecov, artifacts, test quality) - Provide 4 direct reference links to CALLIOPE working examples - Update Phase 2 quick start with CALLIOPE patterns - Clarify rollout strategy for JANUS/MORS (Phase 2b/2c)
- Add tools/update_coverage_threshold.py for automatic threshold updates - Implement coverage ratcheting step in CI (only increases, never decreases) - Rename .github/workflows/ci.yml to ci_tests.yml for consistency with CALLIOPE - Rename docs/testing_infrastructure.md to test_infrastructure.md (shorter, clearer) - Update all references to renamed files in workflows and documentation - Update pyproject.toml with ratcheting mechanism comments - Add test_infrastructure.md to mkdocs.yml navigation Coverage ratcheting ensures sustainable progress: threshold automatically increases when coverage improves on main branch, preventing regression.
…tructure documentation
- Remove unused 'import pytest' from placeholder test files (restructure_tests.sh) - Remove test branches (tl/test_ecosystem_v1/v2) from CI trigger (ci_tests.yml) - Remove unreachable coverage threshold step from macOS job (ci_tests.yml) - Add exception logging to NumPy conversion handler (wrapper.py) - Add explicit error message for missing tomli dependency (update_coverage_threshold.py) - Update SOCRATES cloning guidance to pin commits for supply-chain security (test_infrastructure.md)
Critical fixes: - Fix floating-point coverage comparisons using bc instead of bash operators - Remove unreachable Linux-specific disk space steps from macOS job - Refactor long INSTALL_DEPS command into multiline format - Fix incomplete 'Make coverage badge' step Code quality improvements: - Break long lines in ci_tests.yml (152, 408) to comply with 96-char limit - Wrap long cache key example in documentation (651 chars) - Add shebang to coverage_analysis.sh for consistency - Add period to test_utils.py docstring for consistency Error handling & diagnostics: - Include exception type in update_coverage_threshold.py error message - Include exception type in wrapper.py NumPy conversion logging - Add clarifying comment on ratcheting mechanism equality check Documentation: - Clarify pytest-cov vs coverage run usage patterns in pyproject.toml
- Break SOCRATES cache key onto multiple lines for readability - All cache keys now under 96-character limit per ruff configuration - Applies to both Ubuntu and macOS jobs in ci_tests.yml - Fixes Copilot review comments qwe and qwf
Priority 1 - Critical Fixes: - #38: Update coverage threshold documentation for clarity - #40: Fix chmod +x documentation inconsistency - #41: Ensure missing_count variable initialization (already present) - #43: Make wrapper.py exception handling strict (log.error + raise) - #45: Add tomlkit version constraint (>=0.11.0) - #49, #51: Fix example thresholds from 5% to 30% Priority 2 - Risk Mitigation: - #44: Add git rebase error handling to prevent race conditions - #46: Add bc availability check before floating-point comparisons - #48: Use 'git push origin' instead of token in URL (security) Priority 3 - Code Quality Improvements: - #39: Add comments explaining coverage/pytest-cov dependency overlap - #42: Make import exception handling more specific (from None -> from e) - #50: Add specific exception types (FileNotFoundError, ValueError, KeyError)
…vailable scripts Added documentation for: - Testing & Quality Assurance tools (4 scripts) - External Repository Management tools (6 scripts) - Data & Configuration Tools (3 scripts) - Workflow & Results Management tools (1 script) - Post-Processing & Analysis tools (4 scripts) Each tool includes: - Clear purpose statement - What it does (bullet points) - Usage examples with code blocks - Requirements where applicable - Exit codes or expected outputs
…e tool usage - Update docs/test_infrastructure.md pytest configuration example to match actual pyproject.toml - Clarify that global coverage flags were removed from pytest addopts - Update coverage threshold example from 30% to current 69% (auto-ratcheted) - Improve pyproject.toml comments on coverage[toml] vs pytest-cov distinction - Add coverage tool options to copilot-instructions.md - Document that both 'pytest --cov' and 'coverage run -m pytest' are supported
- Change Python version from 3.13 to 3.12 - Replace pypi-based install-all with developer install for all submodules - Install JANUS, CALLIOPE, ZEPHYRUS as editable packages - Use tools/get_socrates.sh for SOCRATES build - Use AGNI's get_agni.sh for AGNI build (includes wrapper gen and lib build) - Clone SPIDER repo without building (skipping PETSc requirement) - Follow installation.md developer pathway consistently across all modules
- Add test structure validation (bash tools/validate_test_structure.sh) - Add pytest test discovery validation (pytest --collect-only) - Add Codecov integration for coverage reporting - Add HTML coverage artifact uploads (30-day retention) - Fix Python version in auto-ratcheting conditions (3.13 -> 3.12) - Apply validation steps to both test-linux and test-macos jobs These changes ensure workflow compliance with test_infrastructure.md guidelines for test organization, coverage tracking, and ecosystem integration standards.
**Critical Fixes:** 1. Move pytest --collect-only validation AFTER Python and packages installed - Was: Running before Python setup (would fail with 'command not found') - Now: Runs after all dependencies installed for accurate test discovery 2. Add coverage.xml and coverage.html generation - Was: Only generating coverage.json - Now: Generates xml for Codecov, html for artifacts - Codecov upload now has files to upload - HTML artifact upload now has directory to upload **Execution Flow (Both Jobs):** ✅ 1. Test with pytest (coverage run -m pytest) ✅ 2. Generate coverage reports (json, xml, html, report) ✅ 3. Validate test structure and discovery ✅ 4. Report coverage to GitHub summary ✅ 5. Upload coverage to Codecov (./coverage.xml) ✅ 6. Upload HTML artifacts (htmlcov/ 30-day retention) ✅ 7. Auto-ratchet threshold on main (Python 3.12, Linux only) **Consistency:** - Both test-linux and test-macos have identical structure - Coverage reporting now matches test_infrastructure.md expectations - All validation steps run AFTER environment is fully set up - No syntax errors or premature step execution
…ML code block - Convert multiline YAML block scalars to single-line format for cache keys (4 occurrences) - Fix unclosed TOML code block in test_infrastructure.md - Addresses review #579 (review)
Major Changes: - Add Dockerfile with pre-compiled physics modules (SOCRATES, PETSc, SPIDER, AGNI) - Create docker-build.yml workflow (nightly builds at 02:00 UTC) - Create ci-pr-checks.yml workflow (fast PR validation ~10-15 min) - Create ci-nightly-science.yml workflow (deep science validation) - Add 'smoke' pytest marker for quick binary validation - Add comprehensive documentation and example tests Architecture Benefits: - 50+ minute time savings per PR (Python changes) - Smart rebuild: only recompile changed files - Pre-built Docker image reused across all CI workflows - Test stratification: unit → smoke → integration → slow - Nightly comprehensive validation ensures scientific correctness Test Markers: - @pytest.mark.unit: Fast tests with mocked physics (PR checks) - @pytest.mark.smoke: Quick binary validation (PR checks) - @pytest.mark.integration: Multi-module tests (nightly) - @pytest.mark.slow: Full scientific validation (nightly)
- Install Julia 1.11 specifically (required by AGNI Project.toml) - Configure git to use HTTPS instead of SSH (avoid SSH dependency) - Remove PETSc and SPIDER compilation (not needed for tests) - Add test_docker_image.sh for local validation - Image builds successfully: 3.05GB, all modules working
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25c7b611c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+31
to
+35
| @pytest.mark.unit | ||
| def test_atmosphere_temperature_calculation(): | ||
| """Unit test: Test temperature calculation logic (mocked radiation).""" | ||
| # Mock expensive radiative transfer calls | ||
| with patch('proteus.atmosphere.calculate_radiation') as mock_rad: |
There was a problem hiding this comment.
The new unit test mocks proteus.atmosphere.calculate_radiation, but the package has no proteus.atmosphere module (only atmos_chem and atmos_clim exist under src/proteus). When pytest -m unit runs, unittest.mock.patch will attempt to import this path and immediately raise ModuleNotFoundError, causing the unit-test job in ci-pr-checks.yml to fail before any assertions run. Please point the patch at an existing module or remove the mock.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements Docker-based CI/CD architecture to reduce PR testing time from ~60 min to ~10-15 min.
Changes
Testing Plan
Performance Targets
Documentation