Conversation
|
Warning Rate limit exceeded@PythonFZ has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 50 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe pull request migrates molecule handling from rdkit2ase to molify, making it an optional dependency in pyproject.toml. A new molecule_building extension module is introduced providing SMILES-to-atoms conversion, molecular packing, and timing utilities. Dynamic extension loading is added to register these modules if molify is available. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/zndraw/extensions/molecule_building.py (3)
25-29: Add type hints forvisparameter.Per coding guidelines, type hints should be used wherever possible. The
runmethod should have thevisparameter typed.- def run(self, vis, **kwargs): + def run(self, vis: "ZnDraw", **kwargs) -> None:You'll need to add the TYPE_CHECKING import at the top:
if t.TYPE_CHECKING: from zndraw import ZnDraw
55-70: Add type hints and consider adding return type annotation.Same as
AddFromSMILES, therunmethod should include type hints for consistency.- def run(self, vis, **kwargs): + def run(self, vis: "ZnDraw", **kwargs) -> None:
73-85: Consider movingWaitto base modifiers.
Waitdoesn't usemolify- it only uses the standard librarytimemodule. Consider placing it inmodifiers.pyso it's available even without themolifyoptional dependency.Also, add type hints:
- def run(self, vis, **kwargs): + def run(self, vis: "ZnDraw", **kwargs) -> None:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
docs/source/2025/01.rst(1 hunks)pyproject.toml(1 hunks)src/zndraw/extensions/modifiers.py(1 hunks)src/zndraw/extensions/molecule_building.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: If sensible, implement collections.abc interfaces for classes, such as MutableMapping or MutableSequence
Use numpy style docstrings
Docstrings must be concise and to the point
Use type hints wherever possible. Importtyping as tif necessary, but uselist[int|float] | Noneinstead oft.Optional[t.List[int|float]]
Imports should always be at the top of the file
Files:
src/zndraw/extensions/modifiers.pysrc/zndraw/extensions/molecule_building.py
🧬 Code graph analysis (1)
src/zndraw/extensions/molecule_building.py (2)
src/zndraw/extensions/abc.py (2)
Category(10-13)Extension(16-31)src/zndraw/zndraw.py (1)
progress_tracker(1772-1793)
🪛 Ruff (0.14.8)
src/zndraw/extensions/molecule_building.py
25-25: Unused method argument: kwargs
(ARG002)
55-55: Unused method argument: kwargs
(ARG002)
79-79: Unused method argument: kwargs
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: pytest (3.13, ubuntu-latest)
- GitHub Check: pytest (3.12, ubuntu-latest)
- GitHub Check: pytest (3.11, ubuntu-latest)
🔇 Additional comments (5)
docs/source/2025/01.rst (1)
11-11: LGTM!The import path correctly reflects the migration from
rdkit2asetomolify. This aligns with the optional dependency setup inpyproject.toml.pyproject.toml (1)
45-46: LGTM!The optional dependency group is correctly configured. Using
fullis a reasonable convention for including all optional features. Users can install withpip install zndraw[full].src/zndraw/extensions/molecule_building.py (2)
32-42: LGTM!Good use of
Fieldwithjson_schema_extrafor custom UI type hints. The model is concise and well-documented.
88-92: LGTM!The registry follows the same pattern as the base
modifiersdict. Type annotation is correct.src/zndraw/extensions/modifiers.py (1)
436-442: LGTM!The optional import pattern correctly handles the case when
molifyis not installed. Usingtry/except ImportErrorwith silentpassis appropriate for optional features - no need to clutter logs when users haven't installed the optional dependency.
There was a problem hiding this comment.
Pull request overview
This PR restructures molify as an optional dependency and adds new molecule building extensions. By moving molify to an optional extra (full), users can install ZnDraw without chemistry-specific dependencies, improving installation flexibility.
- Moved
molifyfrom required dependencies to optional[full]extra - Added new extensions:
AddFromSMILES,PackBox, andWaitfor molecule building workflows - Updated Dockerfile to include the
fullextra for production builds
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Moved molify from dependencies to optional-dependencies under full extra |
| uv.lock | Updated dependency resolution to mark molify as optional with extra == 'full' marker |
| src/zndraw/extensions/molecule_building.py | New file adding molecule building extensions requiring molify |
| src/zndraw/extensions/modifiers.py | Added optional import of molify_modifiers with ImportError handling |
| docs/source/2025/01.rst | Updated import statement from rdkit2ase to molify |
| Dockerfile | Added --extra full flag to uv sync commands to include molify in Docker builds |
Comments suppressed due to low confidence (4)
src/zndraw/extensions/molecule_building.py:91
- The type annotation
type[Extension]is inconsistent with the mainmodifiersdictionary inmodifiers.pywhich usest.Type[UpdateScene]. Since these modifiers extendExtension(viaUpdateScenebeing a subclass), the type annotation should bedict[str, type[Extension]]ordict[str, t.Type[Extension]]for consistency with the codebase pattern of usingt.Typeimported from typing.
src/zndraw/extensions/molecule_building.py:91 - The new
AddFromSMILES,PackBox, andWaitextensions lack test coverage. Given that the repository has comprehensive test coverage for other modifiers (seetests/test_vis_run_modifier.py), these new extensions should also have tests covering their functionality, edge cases, and error handling.
"""Extensions requiring the molify package."""
import molify
from pydantic import BaseModel, Field
from zndraw.extensions.abc import Category, Extension
class AddFromSMILES(Extension):
"""Add a molecule from SMILES notation."""
category = Category.MODIFIER
smiles: str = Field(
...,
json_schema_extra={
"x-custom-type": "smiles",
"description": "SMILES notation for molecule",
},
)
def run(self, vis, **kwargs):
atoms = molify.smiles2atoms(self.smiles)
vis.append(atoms)
vis.log(f"Added molecule from SMILES: {self.smiles}")
vis.step = len(vis) - 1
class MoleculeSpec(BaseModel):
"""Specification for a molecule in PackBox."""
smiles: str = Field(
...,
json_schema_extra={
"x-custom-type": "smiles",
"description": "SMILES notation for molecule",
},
)
count: int = Field(..., ge=1, description="Number of molecules")
class PackBox(Extension):
"""Pack molecules into a box at specified density."""
category = Category.MODIFIER
molecules: list[MoleculeSpec] = Field(
default=[],
json_schema_extra={"x-custom-type": "smiles-pack-array"},
)
density: float = Field(1.0, ge=0.0)
def run(self, vis, **kwargs):
conformers = [
molify.smiles2conformers(mol.smiles, numConfs=mol.count)
for mol in self.molecules
]
box = molify.pack(
data=conformers,
counts=[mol.count for mol in self.molecules],
density=self.density,
)
vis.append(box)
vis.step = len(vis) - 1
vis.log(
f"Packed box with {len(self.molecules)} molecule types "
f"at density {self.density} kg/m³"
)
molify_modifiers: dict[str, type[Extension]] = {
AddFromSMILES.__name__: AddFromSMILES,
PackBox.__name__: PackBox,
}
src/zndraw/extensions/molecule_building.py:91
- The
Waitextension is defined in this file but not included in themolify_modifiersdictionary. If this extension should be available, it needs to be added to the dictionary. If it's intentionally excluded, consider either removing the class definition or adding a comment explaining why it's not exported.
src/zndraw/extensions/molecule_building.py:77 - The field name
timeshadows the importedtimemodule. This creates ambiguity and could lead to confusion or errors. Consider renaming the field to something likewait_time,duration, orsecondsto avoid the naming conflict with the module.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ] | ||
| box = molify.pack( | ||
| data=conformers, | ||
| counts=[mol.count for mol in self.molecules], | ||
| density=self.density, | ||
| ) | ||
| vis.append(box) | ||
| vis.step = len(vis) - 1 | ||
| vis.log( | ||
| f"Packed box with {len(self.molecules)} molecule types " | ||
| f"at density {self.density} kg/m³" | ||
| ) | ||
|
|
||
|
|
||
| molify_modifiers: dict[str, type[Extension]] = { | ||
| AddFromSMILES.__name__: AddFromSMILES, |
There was a problem hiding this comment.
The run method doesn't validate if the molecules list is empty before processing. When molecules is an empty list (the default), this will create empty conformers and counts lists, which may cause unexpected behavior or errors in molify.pack. Consider adding validation to ensure at least one molecule is specified, or handle the empty case explicitly.
| from zndraw.extensions.molecule_building import molify_modifiers | ||
|
|
||
| modifiers.update(molify_modifiers) | ||
| except ImportError: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except ImportError: | |
| except ImportError: | |
| # Optional extension not available; safe to ignore if molify is not installed. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #810 +/- ##
=======================================
Coverage 78.13% 78.13%
=======================================
Files 152 153 +1
Lines 18327 18357 +30
=======================================
+ Hits 14319 14344 +25
- Misses 4008 4013 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.