Fix fragile cache name property pattern for _xpehh_gwss_cache_name and _ihs_gwss_cache_name#1199
Conversation
…d _ihs_gwss_cache_name Replace abstract @Property @AbstractMethod pattern with safe resolver methods that always return concrete cache name strings. This prevents NotImplementedError crashes from MRO edge cases or version skew, following the same pattern from fix malariagen#1193 for _roh_hmm_cache_name. Changes: - Add _get_xpehh_gwss_cache_name() safe resolver in anopheles.py - Add _get_ihs_gwss_cache_name() safe resolver in anopheles.py - Update call sites to use resolver methods instead of property access - Remove fragile class attribute overrides from species classes (af1, adar1, ag3, adir1, amin1) Fixes malariagen#1151 pattern for two additional cache name properties
There was a problem hiding this comment.
Pull request overview
This PR aims to harden cache-name resolution for ihs_gwss() and xpehh_gwss() by replacing fragile abstract-property access with resolver methods, and updating species resources accordingly to avoid MRO/version-skew NotImplementedError crashes.
Changes:
- Replaced
_xpehh_gwss_cache_name/_ihs_gwss_cache_nameabstract properties with_get_xpehh_gwss_cache_name()/_get_ihs_gwss_cache_name()resolvers. - Updated
ihs_gwss()andxpehh_gwss()to use the new resolver methods. - Removed species-class
_xpehh_gwss_cache_name/_ihs_gwss_cache_nameoverrides (plus minor HTML formatting quote changes).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
malariagen_data/anopheles.py |
Introduces cache-name resolver methods and updates GWSS call sites to use them. |
malariagen_data/amin1.py |
Removes per-species cache-name overrides; minor _repr_html_ string formatting change. |
malariagen_data/ag3.py |
Removes per-species cache-name overrides; minor _repr_html_ string formatting change. |
malariagen_data/af1.py |
Removes per-species cache-name overrides; minor _repr_html_ string formatting change. |
malariagen_data/adir1.py |
Removes per-species cache-name overrides; minor _repr_html_ string formatting change. |
malariagen_data/adar1.py |
Removes per-species cache-name overrides; minor _repr_html_ string formatting change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Use instance-level getattr for proper property descriptor resolution - Add type validation to ensure string returns - Update docstrings from 'should' to 'may' to reflect optional overrides - Add regression tests for MRO edge case handling
|
Hi @kunal-10-cloud, given that it is an extension of #1193 and that you seem to agree with the approach that was taken there, wouldn't it be more efficient to ask @khushthecoder to apply the same solution to these other 2 cases? |
|
Hi @jonbrenas, actually @khushthecoder and myself decided to collaborate on this issue by splitting the work amongst us |
|
Hi @jonbrenas I definitely see why having one person do it all might seem more efficient on paper! However, as @kunal-10-cloud mentioned, we are working on these issues together locally. Our goal in splitting the work was to speed up our contributions to the repository by tackling different parts of the codebase in parallel and learning the architecture together. He is completely synced with the exact pattern and implementation from my PR (#1193), so the code style and approach will be perfectly consistent. Since his PR (#1199) is already open, tested, and ready to go, it would mean a lot to both of us if we could keep them separate so we can both get the experience of having our individual contributions reviewed and merged. We're really enjoying contributing to the project and hope this split approach works for you! |
…alariagen#1199) Following the pattern from malariagen#1193, add _get_xpehh_gwss_cache_name() and _get_ihs_gwss_cache_name() resolver methods to safely handle cache name resolution with fallback defaults.
|
HI @jonbrenas, i have updated my approach and aligned myself with the #1193 keeping the codebase consistent |
Description
This PR fixes a fragile pattern in
_xpehh_gwss_cache_nameand_ihs_gwss_cache_namethat could causeNotImplementedErrorcrashes under MRO edge cases or version skew. The fix applies the same safe resolver pattern established in #1193 for_roh_hmm_cache_name.Fixes: #1198
Changes
anopheles.py
from abc import abstractmethod(no longer needed)_xpehh_gwss_cache_nameproperty with_get_xpehh_gwss_cache_name()resolver method_ihs_gwss_cache_nameproperty with_get_ihs_gwss_cache_name()resolver methodihs_gwss()call site (line ~742) to use_get_ihs_gwss_cache_name()xpehh_gwss()call site (line ~1263) to use_get_xpehh_gwss_cache_name()Species Classes (af1.py, adar1.py, ag3.py, adir1.py, amin1.py)
_xpehh_gwss_cache_nameclass attribute override_ihs_gwss_cache_nameclass attribute overrideType of Change
Testing
[X] Python syntax validation passed
[X] Ruff linting passed
[X] Ruff formatting passed
[X] Mypy type checking passed (no issues found)
[X] Pre-commit hooks passed
Related Issues
Fixes #1151 pattern for two additional cache name properties. Implements the same solution pattern established in #1193 for
_roh_hmm_cache_name.Checklist