Modern Python + rdkit support (build on #40) - #42
Open
pbio wants to merge 4 commits into
Open
Conversation
* Bump python upper bound to <3.14 in pyproject.toml and env-dev.yml. * Use per-Python-version pins for numpy and rdkit so 3.9-3.12 users keep numpy<2 / rdkit ^2023.9.1 (no degradation), while 3.13 picks up numpy>=2.1 / rdkit>=2024.9.1 (the first versions with cp313 wheels). * SCScore._make_fingerprint: use DataStructs.ConvertToNumpyArray instead of np.array(bitvect, dtype=float). The latter segfaults under rdkit>=2026 + numpy>=2. * TemplateMolecule.atom_properties: swallow Boost.Python.ArgumentError from RDKit getters whose signatures changed in rdkit>=2024 (e.g. Atom.GetValence now requires a ValenceType arg). * Mark a handful of template/hash tests xfail on Python 3.13 because rdkit>=2024 produces different fingerprint hashes / template SMARTS for those inputs. Tests: 287 pass / 0 fail on Python 3.12 (baseline preserved); 282 pass / 6 xfail / 3 xpass on Python 3.13.
The three tests previously gated on `_NEW_RDKIT` asserted rdkit-specific implementation details rather than the properties they were meant to verify. Rework each so a single assertion works on any rdkit version: * test_template_creation_different_radius: compare template graphs (atoms + bonds keyed on atom-map number) rather than the exact SMARTS string, which depends on rdkit's writer branch order. * test_hash_from_smiles / test_hash_from_smarts: check the hash function's invariants (deterministic, distinguishing, well-formed SHA224) instead of pinning a specific hex value derived from rdkit's canonical writer. * test_template_equality (Al row): swap the [AlH3] / [MolecularAI#13&H3] pair for [CH3] / [MolecularAI#6&H3]. The test's intent is element-symbol vs. atomic-number SMARTS notation equivalence; Al was incidental and its valence is now rejected by rdkit>=2024's property cache. * test_template_equality_chiral: replace the ambiguous stereocenter (three identical -C substituents, distinguished only by atom-map numbers) with a genuine [C@H] vs [C@@h] enantiomer pair on an asymmetric scaffold. The old assertion held only because rdkit<2024 treated atom-map numbers as chirality-distinguishing. Full suite passes on rdkit 2023.9.6 and 2024.9.3. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Apply the four changes CKannas requested in the review of MolecularAI#40: * rdkit (Python <3.13): loosen upper bound from <2024.0.0 to <2024.9.1 so the 2024.3.x series is available before Python 3.13 wheels start. * numpy (Python >=3.13): lower floor from >=2.1.0,<3.0.0 to >=2.0.0 (rdkit >=2024.9.1 is compatible with NumPy 2.0). * scipy: split into <3.13 -> >=1.11.4,<1.14.1 and >=3.13 -> >=1.14.1 since scipy 1.14 dropped Python 3.9 support. * onnxruntime: split into <3.12 -> <1.17.0 and >=3.12 -> >=1.17.0. Also pin poetry to 2.4.0 in env-dev.yml per the same review. Regenerated poetry.lock against the new constraints. Full test suite passes on rdkit 2023.9.6, 2024.3.6, 2024.9.3, 2025.9.6, and 2026.3.5. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The introspection loop in `TemplateMolecule.atom_properties` walks every `Get*` method on the atom object. A prior fix carried a hardcoded `_ZERO_ARG_GETTER_BLOCKLIST` to skip `GetValence`, whose signature was said to change in rdkit>=2024. That version was wrong: `GetValence` was actually introduced (with a required `ValenceType` argument) in rdkit 2025.03.1. In earlier releases it isn't a method on `Atom` at all, so it never appears in `dir(atom)` and the loop skipped it naturally. Rather than keep chasing rdkit-version boundaries, catch `TypeError` around the reflective call. `Boost.Python.ArgumentError` (raised by rdkit's bindings on argument mismatches) is a `TypeError` subclass, so this covers `GetValence` and any future signature change with no per-version code and no imports of Boost.Python internals. Verified against the full test suite on rdkit 2023.9.6, 2024.3.6, 2024.9.3, 2025.9.6, and 2026.3.5. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
Extends #40 (Edison's Python 3.13 support work) with three additional commits that address the review feedback from @CKannas and remove rdkit-version-gated test logic in favor of behavior-based checks.
Included in this PR:
2059677) is preserved intact: Python 3.13 support, per-Python-version rdkit/numpy pins, SCScore numpy-2 fix, and the initialatom_propertiesfix for theGetValencesignature change.8fcf6d6) — Drop rdkit version gates in favor of behavior-based tests: replaces all_NEW_RDKITconditionals intests/test_reaction.pyandtests/test_template.py. The parametrized template test now compares reaction graphs (atoms + bonds keyed on atom-map number) rather than exact SMARTS strings. The hash tests now assert on invariants (determinism, distinguishing, well-formed SHA224) rather than pinned hex values. The[AlH3]/[#13&H3]equality row was swapped for[CH3]/[#6&H3]since Al was incidental to the notation-equivalence check. The chiral fingerprint test was rewritten to use a genuine[C@H]vs[C@@H]enantiomer pair on a scaffold with four distinguishable substituents, which is chemically well-defined across all rdkit versions.32eabd7) — Address version-constraint feedback from PR review: applies exactly the four version splits @CKannas requested (rdkit<2024.9.1, numpy>=2.0.0, per-Python scipy split, per-Python onnxruntime split) plus thepoetry=2.4.0pin inenv-dev.yml.14f8467) — Replace GetValence blocklist with TypeError catch in atom_properties:Boost.Python.ArgumentErrorinherits fromTypeError, so catching that in the reflectiveGet*loop removes the need for a hardcoded blocklist and works forward against any future signature change. Also updates the code comment —GetValencewas actually introduced in rdkit 2025.03.1 (not 2024).Test plan
Verified with the full test suite (
pytest --deselect tests/test_rinchi.py; the two rinchi failures are pre-existing on macOS and unrelated) on Python 3.12 against five rdkit versions across four major series:<2024.9.1cap)Zero failures anywhere. No version-conditional test logic remains in either test file.