Skip to content

Remove python level MMFF prop shim in favor of direct C++#138

Open
scal444 wants to merge 3 commits intoNVIDIA-Digital-Bio:mainfrom
scal444:one_more_fix
Open

Remove python level MMFF prop shim in favor of direct C++#138
scal444 wants to merge 3 commits intoNVIDIA-Digital-Bio:mainfrom
scal444:one_more_fix

Conversation

@scal444
Copy link
Copy Markdown
Collaborator

@scal444 scal444 commented Apr 24, 2026

The weird python cache wasn't actually working for all use cases, which was being hidden in unit tests, unfortunately, due to the call to capture_mmff_settings.

The difficulty in general was that this class isn't exposed via public API to the python interface on the rdkit side, but since we can see it, we can create our own quick wrapper.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 24, 2026

Greptile Summary

This PR removes the Python-level WeakKeyDictionary MMFF settings cache (which silently failed for externally-configured MMFFMolProperties objects) and replaces it with a C++ helper, buildMMFFPropertiesFromRDKit, that reads settings directly from the underlying RDKit::MMFF::MMFFMolProperties via a layout-compatible shim. The approach is well-documented, the pre-existing ODR/ABI fragility is acknowledged with warnings and a TODO pointing at the upstream RDKit issue, and a targeted new test demonstrates the fix works for the exact failure mode described.

Confidence Score: 5/5

Safe to merge — fixes a real bug, no new correctness issues introduced, pre-existing platform limitations are clearly documented

No P0 or P1 issues found. The ODR and ABI fragility concerns raised in the previous review thread have been addressed with explicit \warning docs, a static_assert, and a TODO tracking the upstream fix. The new test directly exercises the formerly broken code path.

No files require special attention

Important Files Changed

Filename Overview
nvmolkit/mmff_python_utils.h Adds ForceFields::PyMMFFMolProperties shim and buildMMFFPropertiesFromRDKit; relies on Linux ODR symbol merging (documented, previously reviewed, with static_assert guard and TODO)
nvmolkit/_mmff_bridge.py Removes the fragile Python-level WeakKeyDictionary settings cache and delegates directly to _batchedForcefield.buildMMFFPropertiesFromRDKit; clean simplification
nvmolkit/batchedForcefield.cpp Adds Boost.Python binding for buildMMFFPropertiesFromRDKit with correct arg names and docstring
nvmolkit/tests/test_batched_forcefield.py Removes capture_mmff_settings dependency; adds test_mmff_batched_forcefield_reads_externally_configured_properties covering the exact bug the PR fixes
nvmolkit/tests/test_mmff_optimization.py Removes capture_mmff_settings import; replaces capture_mmff_settings call with a direct return mmff_props

Reviews (3): Last reviewed commit: "Add explicit support test" | Re-trigger Greptile

Comment thread nvmolkit/mmff_python_utils.h
Comment thread nvmolkit/mmff_python_utils.h
Document the Linux/ELF symbol-resolution dependency and the upstream
RDKit issue tracking the permanent fix. Add a static_assert guarding
against accidental layout changes to the shim itself.
@scal444 scal444 requested a review from evasnow1992 April 24, 2026 21:49
Copy link
Copy Markdown
Collaborator

@evasnow1992 evasnow1992 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me. Thank you for fixing this bug. May benefit from adding one test for testing building properties with raw rdForceFieldHelpers.MMFFGetMoleculeProperties(mol), then configuring with Set*Term() calls, and handing to MMFFBatchedForcefield, to see if the previous broken path works now. Not a blocker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants