Run the full test suite in WebAssembly CI - #783
Conversation
Add a test_wasm_suite job that runs the whole pytest suite in a Pyodide virtual environment (pyodide-build + node), with example files seeded natively into the pooch cache via DFS_DATA_DIR so no network access is needed inside the runtime. Tests that spawn threads or processes can never pass on WebAssembly, so they are tagged with a new 'concurrency' marker and deselected along with 'network' tests in the wasm job only. Also fix three portability bugs the wasm (32-bit) run exposed: - _float_array_to_ns overflowed for integer input arrays on platforms where the default integer is 32 bits. - The ProdML writer stored Count/StartIndex/NumberOfLoci/StartLocusIndex HDF5 attrs as platform-native ints (int32 on wasm32) instead of int64. - dascore.utils.progress.track crashed on WebAssembly because rich progress needs a refresh thread; it now falls back to plain iteration.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesWASM test execution
ProdML attribute typing
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 4e996b9ac4
ℹ️ 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".
| for name in get_fetcher().registry: | ||
| fetch(name) |
There was a problem hiding this comment.
On every cold cache, this iterates the complete Pooch registry, including whale_1.hdf5, which the repository explicitly classifies in LARGE_REGISTRY_FILES and excludes from both the test parametrization and the normal .github/scripts/cache_test_data.py priming path. Consequently, the new 30-minute job downloads a large external dataset that no selected test uses, risking a timeout and unnecessarily bloating the Actions cache; seed via the existing script or get_registry_df(exclude_large=True) instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3e19de6 — the step now calls .github/scripts/cache_test_data.py (exclude_large=True) instead of walking the raw registry, same as the mamba install action. Confirmed real: the first cold-cache run did download whale_1.hdf5, and nothing selected reads it (SKIP_DATA_FILES in test_common_io.py plus the exclude_large=True parametrization). Seeded set drops from 50 files to 49.
| - name: Run test suite in WebAssembly | ||
| run: > | ||
| .venv-pyodide/bin/python -m pytest tests | ||
| -m "not network and not concurrency" |
There was a problem hiding this comment.
This marker expression still selects tests/test_core/test_spool.py::TestMap::test_process_client, even though that test constructs a ProcessPoolExecutor. On WebAssembly, ProcessPoolExecutor raises NotImplementedError from its system-limit check because multiprocessing.synchronize is unavailable, while the proc_client fixture catches only PermissionError, OSError, and RuntimeError; the advertised WASM suite therefore errors instead of skipping this test. Mark the test as concurrency or explicitly handle NotImplementedError in the fixture.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this one. The full wasm suite went green on CI before any fix: 7,836 passed / 247 skipped / 128 deselected / 0 failed, 0 errors (run 30188499383). test_process_client neither errored nor failed there, so the predicted NotImplementedError from _check_system_limits does not surface on this Pyodide build.
Marking it concurrency would deselect a test that currently passes on wasm, which costs coverage for no benefit. Leaving it selected also means CI tells us if a future Pyodide release changes this, rather than hiding it behind a marker.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #783 +/- ##
===========================================
- Coverage 100.00% 99.98% -0.02%
===========================================
Files 164 164
Lines 17589 17663 +74
===========================================
+ Hits 17589 17661 +72
- Misses 0 2 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
DFS_DATA_DIR is a pooch-level override that beats the configured cache directory, which broke the two downloader tests that set it via config_context. XDG_CACHE_HOME only shifts the default location, so the seeded cache is still shared between the native and wasm runs while config overrides keep working.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/test_wasm.yml (1)
47-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPyodide version differs between the two WASM jobs.
test_wasmpinspyodide@314.0.2(npm) while the newtest_wasm_suitejob usesPYODIDE_VERSION: '314.0.3'. Both jobs validate the same wheel against WebAssembly; running them against different Pyodide versions risks inconsistent results and makes failures harder to reason about. Consider aligning both jobs on the same Pyodide version unless the difference is deliberate.Also applies to: 60-60
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test_wasm.yml around lines 47 - 48, Align the Pyodide version used by the existing test_wasm job with the PYODIDE_VERSION configured for test_wasm_suite, updating the npm install step around “Install Pyodide” so both WASM jobs validate against the same release.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test_wasm.yml:
- Around line 68-71: Update the actions/checkout@v4 step in test_wasm_suite to
explicitly disable persisted credentials by setting persist-credentials to
false, while preserving the existing fetch-tags and fetch-depth settings.
---
Nitpick comments:
In @.github/workflows/test_wasm.yml:
- Around line 47-48: Align the Pyodide version used by the existing test_wasm
job with the PYODIDE_VERSION configured for test_wasm_suite, updating the npm
install step around “Install Pyodide” so both WASM jobs validate against the
same release.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 23098063-8f46-42ee-a312-3834623d56df
📒 Files selected for processing (18)
.github/workflows/test_wasm.ymldascore/io/prodml/utils.pydascore/utils/progress.pydascore/utils/time.pypyproject.tomltests/test_core/test_directory_spool.pytests/test_core/test_spool.pytests/test_imports.pytests/test_io/test_index/test_catalog.pytests/test_io/test_index/test_index_edge_cases.pytests/test_io/test_index/test_schema.pytests/test_io/test_index/test_union.pytests/test_io/test_io_core.pytests/test_units.pytests/test_utils/test_config.pytests/test_utils/test_misc.pytests/test_utils/test_namespace.pytests/test_utils/test_progress.py
The smoke test pinned pyodide@314.0.2 via npm while the new full-suite job installed the 314.0.3 xbuildenv, so the two jobs validated different releases. Both now read PYODIDE_VERSION from one workflow-level env.
4dc0369 to
173cee7
Compare
|
CodeRabbit review addressed:
|
The inline seeding loop walked the whole Pooch registry, including whale_1.hdf5, which LARGE_REGISTRY_FILES excludes from the test parametrization and no selected test reads. Reuse .github/scripts/cache_test_data.py, which the mamba action already uses, so the job skips the large download and the Actions cache stays small.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/test_wasm.yml (1)
91-96: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPin the
pyodide-buildtoolchain version.The Pyodide runtime is pinned to
314.0.3, but this install resolves the latestpyodide-buildon every run. Pin an exact, tested compatible version or install it from a lockfile to prevent future CLI/xbuildenv drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test_wasm.yml around lines 91 - 96, Update the “Create Pyodide virtual environment” step to install an exact, tested pyodide-build version compatible with the pinned PYODIDE_VERSION 0.26.3, or source that version from the project’s lockfile, before invoking pyodide xbuildenv and venv commands.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/test_wasm.yml:
- Around line 91-96: Update the “Create Pyodide virtual environment” step to
install an exact, tested pyodide-build version compatible with the pinned
PYODIDE_VERSION 0.26.3, or source that version from the project’s lockfile,
before invoking pyodide xbuildenv and venv commands.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9fefec5e-fa33-48d3-accb-4f0d9a9d0ed8
📒 Files selected for processing (1)
.github/workflows/test_wasm.yml
The runtime was pinned to 314.0.3 but pyodide-build resolved to whatever was newest at run time, so a toolchain release could change the xbuildenv underneath a pinned runtime. Pin 0.37.0, the version the last green run installed.
|
Latest CodeRabbit nitpick addressed in 625da47: |
1 participant
Description
Follow-up to #756: instead of only the smoke script, run the entire pytest suite in a real Pyodide (WebAssembly) runtime on CI.
The new
test_wasm_suitejob builds the universal wheel, creates a Pyodide 314.0.3 virtual environment with pyodide-build, seeds the example-data cache natively with.github/scripts/cache_test_data.py(viaXDG_CACHE_HOME, cached on the registry hash) so the wasm run needs no network, and runspytest tests -m "not network and not concurrency".WebAssembly cannot start threads or processes, so tests that inherently spawn them are tagged with a new
concurrencymarker (28 tests: thread/process pool clients,run_in_threadsconcurrency tests, subprocess-based lazy-import tests, and rich-progress tests) and deselected only in the wasm job. All other 7,800+ tests run and pass.Running the suite on wasm32 exposed three genuine portability bugs, fixed here:
dascore.utils.time._float_array_to_nsoverflowed for integer input arrays on platforms where the default numpy integer is 32-bit (e.g.to_timedelta64(20)returned -1.47 s); integers are now widened to int64 before the seconds→ns multiply. This alone fixed 37 tests on wasm.Count/StartIndex/NumberOfLoci/StartLocusIndexHDF5 attributes as platform-native ints (int32 on wasm32); they are now explicitnp.int64.dascore.utils.progress.trackcrashed any progress-reporting operation (e.g.spool.update()) on WebAssembly because rich progress spawns a refresh thread; it now falls back to plain iteration onemscripten/wasi.Verified in CI (
test_wasm_suite): 7,836 passed / 247 skipped / 128 deselected / 0 failed in 4m39s, and the full native suite passes unchanged.Changelog
dc.to_datetime64anddc.to_timedelta64convert integer arrays exactly. Integer input was routed throughnp.rint, whose float64 intermediate loses precision past 2^53 ns, so times beyond about 104 days from the epoch were off by up to 512 ns; the multiply also overflowed a 32-bit default integer under WebAssembly.Checklist
I have (if applicable):
Summary by CodeRabbit
concurrencytest marker and applied it across relevant tests/configuration for clearer selection.