feat: add naming feature from rdkit mol obj - #43
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe naming pipeline now accepts existing RDKit molecules directly, converts them into internal graphs without mutating callers, supports tracing, analysis, verification, and mixed-input batches, and exposes the new entry points through the public API and README. ChangesRDKit molecule naming
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant NamingEngine
participant read_rdkit_mol
participant MoleculeGraph
Caller->>NamingEngine: name_rdkit_mol(rdkit_mol)
NamingEngine->>read_rdkit_mol: convert RDKit Mol
read_rdkit_mol->>MoleculeGraph: build prepared Molecule
NamingEngine->>MoleculeGraph: name connected components
MoleculeGraph-->>NamingEngine: generated name
NamingEngine-->>Caller: return name
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Reviewer's GuideAdd RDKit molecule-based naming and analysis APIs, refactor the naming engine to accept either SMILES or RDKit Mol input (including mixed batches), and extend graph I/O to safely build internal molecule graphs from RDKit molecules, while updating public exports, version, docs, and tests. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
AdrianM0
marked this pull request as ready for review
July 21, 2026 21:15
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new public APIs and engine methods all type
rdkit_molasAny; consider tightening these annotations (e.g., toChem.Mol | None) so misuse is caught earlier and IDEs can provide better assistance. - In
name_many, theIterable[str | Any]and_request_forlogic will treat any non-string item as an RDKit molecule; it may be safer to explicitly check forChem.Mol(or a protocol) rather than a broadAnyto avoid misinterpreting other object types. - The multiprocessing worker
_name_one_for_workernow acceptstuple[str | Any, bool, bool, bool]; making this signature consistent with the narrowed RDKit types and documenting the expected item types would reduce ambiguity around what is pickled and sent to workers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new public APIs and engine methods all type `rdkit_mol` as `Any`; consider tightening these annotations (e.g., to `Chem.Mol | None`) so misuse is caught earlier and IDEs can provide better assistance.
- In `name_many`, the `Iterable[str | Any]` and `_request_for` logic will treat any non-string item as an RDKit molecule; it may be safer to explicitly check for `Chem.Mol` (or a protocol) rather than a broad `Any` to avoid misinterpreting other object types.
- The multiprocessing worker `_name_one_for_worker` now accepts `tuple[str | Any, bool, bool, bool]`; making this signature consistent with the narrowed RDKit types and documenting the expected item types would reduce ambiguity around what is pickled and sent to workers.
## Individual Comments
### Comment 1
<location path="src/openclatura/engine.py" line_range="415-416" />
<code_context>
)
def _run_parallel(
- smiles_list: list[str],
+ smiles_list: list[str | Any],
*,
include_trace: bool,
</code_context>
<issue_to_address>
**issue (bug_risk):** Multiprocessing with RDKit molecules is likely to fail because RDKit Mol objects are not picklable.
`name_many` now accepts RDKit `Mol` instances in `smiles_iter`, and `_run_parallel` passes these directly to `multiprocessing.Pool.map` via `_name_one_for_worker`. Because RDKit `Mol` objects are not picklable, any parallel run (`processes != 1`) with molecules will likely fail with pickling errors. To avoid this, either restrict parallel mode to SMILES-only inputs (and enforce/document that), or convert `Mol` objects to a picklable form (e.g., SMILES) before sending them to worker processes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
AdrianM0
changed the title
![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 (2)
src/openclatura/engine.py (1)
311-312: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrace phase text is now misleading for RDKit-molecule input.
_analyzenow accepts a pre-builtMoleculegraph from either SMILES or an RDKitMol(per the_prepare_inputrefactor), but the firstTracePhase.PARSEdecision it emits still hardcodes"parsed SMILES"/"RDKit parsing populated..."and recordsdata={"smiles": smiles, ...}— foranalyze_rdkit_mol/name_rdkit_mol_with_tracecalls withoutverify_opsin,smilesis"", so the trace claims a SMILES was parsed with an empty SMILES value, which is confusing for consumers of the explainability trace (a headline feature of this PR).Consider branching the wording (e.g.
"parsed RDKit molecule"whenrequest.rdkit_molwas the source) so the trace accurately reflects the input path, sinceanalyze_rdkit_molis explicitly meant to mirroranalyze_smiles's explainability contract.🤖 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 `@src/openclatura/engine.py` around lines 311 - 312, Update the initial TracePhase.PARSE decision in _analyze to distinguish RDKit-molecule input from SMILES input, using wording and trace data that accurately identify the source; preserve the existing SMILES details for SMILES analysis and emit RDKit-specific text when request.rdkit_mol is the source, including cases where smiles is empty.src/openclatura/graph_io.py (1)
28-63: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a stereo regression test for the explicit-H path. The current explicit-H test only exercises benzoic acid, so it doesn’t cover a stereocenter whose configuration depends on an explicit hydrogen. Add a case built from
Chem.AddHs/MolToMolBlock/MolFromMolBlockand assertname_rdkit_molmatches the SMILES path.🤖 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 `@src/openclatura/graph_io.py` around lines 28 - 63, Add a regression test for the explicit-hydrogen path in read_rdkit_mol using a stereocenter whose configuration depends on an explicit hydrogen. Build the molecule through Chem.AddHs, serialize with MolToMolBlock, reload with MolFromMolBlock, and assert name_rdkit_mol produces the same result as the corresponding SMILES-based molecule.
🤖 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 `@README.md`:
- Around line 96-116: Update the “Naming an existing RDKit molecule” example so
the calls to name_mol and name_many use a separately defined, known-valid
molecule rather than the for-loop variable mol. Keep the SD supplier loop
focused on naming each non-None record and ensure the batch example still
demonstrates both RDKit-molecule and SMILES inputs.
---
Nitpick comments:
In `@src/openclatura/engine.py`:
- Around line 311-312: Update the initial TracePhase.PARSE decision in _analyze
to distinguish RDKit-molecule input from SMILES input, using wording and trace
data that accurately identify the source; preserve the existing SMILES details
for SMILES analysis and emit RDKit-specific text when request.rdkit_mol is the
source, including cases where smiles is empty.
In `@src/openclatura/graph_io.py`:
- Around line 28-63: Add a regression test for the explicit-hydrogen path in
read_rdkit_mol using a stereocenter whose configuration depends on an explicit
hydrogen. Build the molecule through Chem.AddHs, serialize with MolToMolBlock,
reload with MolFromMolBlock, and assert name_rdkit_mol produces the same result
as the corresponding SMILES-based molecule.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 099f3e9c-f794-4768-9c8c-7640bc3b6921
📒 Files selected for processing (6)
README.mdsrc/openclatura/__init__.pysrc/openclatura/engine.pysrc/openclatura/graph_io.pysrc/openclatura/namer.pysrc/openclatura/tests/test_public_api.py
Comment on lines
+96
to
+116
| ### Naming an existing RDKit molecule | ||
|
|
||
| If you already hold an `rdkit.Chem.rdchem.Mol` — from an SD file, a reaction, | ||
| or an earlier step in a pipeline — skip the SMILES round-trip: | ||
|
|
||
| ```python | ||
| from rdkit import Chem | ||
| from openclatura import name_rdkit_mol, name_mol, name_many | ||
|
|
||
| for mol in Chem.SDMolSupplier("compounds.sdf"): | ||
| if mol is not None: | ||
| print(name_rdkit_mol(mol)) # -> 'benzoic acid' | ||
|
|
||
| name_mol(mol) # typed NamingResult, as `name` | ||
| name_many([mol, "CCO"]) # batches take either form | ||
| ``` | ||
|
|
||
| The input molecule is never modified, explicit hydrogens (as SD files usually | ||
| carry them) are handled, and `name_rdkit_mol_with_trace` / `analyze_rdkit_mol` | ||
| mirror their SMILES counterparts. | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Example reuses the for loop's leaked variable, which can be None or the wrong molecule.
for mol in Chem.SDMolSupplier("compounds.sdf"):
if mol is not None:
print(name_rdkit_mol(mol)) # -> 'benzoic acid'
name_mol(mol) # typed NamingResult, as `name`
name_many([mol, "CCO"]) # batches take either formAfter the loop, mol is whatever the supplier yielded on its last iteration — including None if the last record failed to parse — not a value guarded by the if mol is not None check. Copy-pasting this as-is can silently call name_mol/name_many on None or an unintended molecule.
📝 Proposed fix
-for mol in Chem.SDMolSupplier("compounds.sdf"):
- if mol is not None:
- print(name_rdkit_mol(mol)) # -> 'benzoic acid'
-
-name_mol(mol) # typed NamingResult, as `name`
-name_many([mol, "CCO"]) # batches take either form
+mols = [m for m in Chem.SDMolSupplier("compounds.sdf") if m is not None]
+for mol in mols:
+ print(name_rdkit_mol(mol)) # -> 'benzoic acid'
+
+name_mol(mols[0]) # typed NamingResult, as `name`
+name_many([mols[0], "CCO"]) # batches take either form🤖 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 `@README.md` around lines 96 - 116, Update the “Naming an existing RDKit
molecule” example so the calls to name_mol and name_many use a separately
defined, known-valid molecule rather than the for-loop variable mol. Keep the SD
supplier loop focused on naming each non-None record and ensure the batch
example still demonstrates both RDKit-molecule and SMILES inputs.
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 by Sourcery
Introduce RDKit molecule–based naming and analysis alongside existing SMILES APIs, allowing callers to bypass SMILES round-trips while maintaining compatibility with verification and batch processing workflows.
New Features:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Documentation