Fixes after merge of #154.#155
Conversation
arokem
commented
Jan 20, 2026
arokem
commented
- Make onnxruntime optional and skip dependent tests if not installed.
- Install all the requirements on GitHub action to test source dist.
- Make onnxruntime optional and skip dependent tests if not installed. - Install all the requirements on GitHub action to test source dist.
There was a problem hiding this comment.
Pull request overview
This PR makes onnxruntime an optional dependency and ensures tests dependent on it are skipped when not installed, while also updating the GitHub Actions workflow to install all optional dependencies when testing the source distribution.
Changes:
- Made onnxruntime import optional with try/except block and added skip decorators to dependent tests
- Updated GitHub workflow to install source distribution with all optional dependencies
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| AFQ/tests/test_nn.py | Added optional import handling for onnxruntime and skip decorators for tests that require it |
| .github/workflows/publish-to-test-pypi.yml | Modified pip install command to include [all] extras for comprehensive testing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from AFQ.nn.multiaxial import run_multiaxial | ||
|
|
||
|
|
||
| @npt.dec.skipif(not has_onnx, reason="onnxruntime is not installed") |
There was a problem hiding this comment.
The decorator npt.dec.skipif is deprecated in NumPy. Since pytest is already used in other test files in this codebase, consider using pytest.mark.skipif instead for consistency and to avoid using deprecated APIs. This would require importing pytest at the top of the file.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| npt.assert_(chopped_brain.get_fdata().sum() > 200000) | ||
|
|
||
|
|
||
| @npt.dec.skipif(not has_onnx, reason="onnxruntime is not installed") |
There was a problem hiding this comment.
The decorator npt.dec.skipif is deprecated in NumPy. Since pytest is already used in other test files in this codebase, consider using pytest.mark.skipif instead for consistency and to avoid using deprecated APIs. This would require importing pytest at the top of the file.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
In what circumstance would we not want to run the onnx tests? It is already optional in the main code. |
|
|
|
@36000 : This is ready for a review/merge. We won't really know if it works until it's merged and the workflow is triggered, but 🤞 |
- Back off installation of sdist with all dependencies. - Tests should still be fine because onnxruntime now optional.
