Conversation
There was a problem hiding this comment.
Pull request overview
This PR makes the numba dependency optional to support environments where numba is unavailable or undesired, introducing a new alphabase[full] installation option.
Changes:
- Introduced a
numba_wrappermodule that provides fallback implementations when numba is not installed - Moved
numbafrom core requirements to optionalfulldependencies - Added test markers to skip numba-dependent tests when the package is not available
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| alphabase/numba_wrapper.py | New compatibility module providing fallback decorators and types when numba is unavailable |
| alphabase/constants/atom.py | Replaced direct numba imports with wrapper imports |
| alphabase/constants/isotope.py | Replaced direct numba imports with wrapper imports |
| alphabase/constants/modification.py | Replaced direct numba imports with wrapper imports |
| alphabase/peptide/fragment.py | Replaced direct numba imports with wrapper imports |
| alphabase/peptide/precursor.py | Replaced direct numba imports with wrapper imports |
| alphabase/protein/fasta.py | Replaced direct numba imports with wrapper imports |
| alphabase/protein/lcp_digest.py | Replaced direct numba imports with wrapper imports |
| alphabase/psm_reader/alphapept_reader.py | Removed numba decorators that provided no benefit |
| alphabase/psm_reader/maxquant_reader.py | Removed numba decorators that provided no benefit |
| alphabase/spectral_library/annotate.py | Replaced direct numba imports with wrapper imports |
| alphabase/spectral_library/translate.py | Replaced direct numba imports with wrapper imports |
| requirements/requirements.txt | Removed numba from core dependencies with clarifying comments |
| requirements/requirements_loose.txt | Removed numba and contextlib2 from loose requirements |
| requirements/requirements_full.txt | New file pinning numba version for full installation |
| requirements/requirements_full_loose.txt | New file for unpinned numba requirement |
| pyproject.toml | Added full and full-stable optional dependencies |
| tests/integration/conftest.py | Added pytest marker for skipping tests requiring numba |
| tests/unit/constants/test_isotope.py | Added requires_numba marker |
| tests/unit/constants/test_modification_functions.py | Added requires_numba marker |
| tests/unit/protein/test_fasta.py | Added requires_numba marker |
| tests/unit/spectral_library/test_annotate.py | Added requires_numba marker |
| tests/test_fragment.py | Added requires_numba marker |
| nbs_tests/conftest.py | New conftest to skip notebook tests when numba unavailable |
| nbs_tests/test_thread.ipynb | Removed test notebook that directly used numba |
| README.md | Updated installation instructions for optional numba dependency |
| .github/workflows/pip_installation.yml | Added test jobs for both with and without numba installation |
| .github/workflows/branch-checks.yaml | Updated to test both full and not-full installations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
| @numba.njit | ||
| def truncate_isotope(isotopes: np.ndarray, mono_idx: int) -> tuple: |
There was a problem hiding this comment.
The @numba.njit decorator was removed from truncate_isotope, but this function is still called from numba-compiled code in reset_elements. When numba is installed, numba cannot compile calls to undecorated functions. This will cause compilation errors in reset_elements when numba is available.
There was a problem hiding this comment.
yikes.. fixed this using a conditional decorator
This would be the required changes to have the numba dependency optional. Let me know what you think.
Note that in the first two commits, I removed numba decorators. According to Claude Code, they should not have given any benefit in the first place.
Called the extra "full", as next steps could be to make
rdkitoptional. But maybe more fine-granular choice of dependencies is desired here?