Fall back through toolkits for InChI conversion methods#2202
Open
gaoflow wants to merge 1 commit into
Open
Conversation
`Molecule.to_inchi`, `to_inchikey`, and `from_inchi` called `ToolkitRegistry.call` without `raise_exception_types=[]`, so they used the default behavior of re-raising the first toolkit's exception instead of falling back to the next registered toolkit. For example, OpenEye refuses InChI generation for molecules larger than 1024 atoms, but the registry never fell back to RDKit, which supports them. Pass `raise_exception_types=[]` at these call sites, matching the convention already used by the rest of the Molecule API, so a higher-precedence toolkit that raises is skipped in favor of the next one. A single aggregate ValueError is still raised if every toolkit fails. Closes openforcefield#2172.
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.
Description
Molecule.to_inchi,Molecule.to_inchikey, andMolecule.from_inchicallToolkitRegistry.call(...)withoutraise_exception_types=[], so they get the default behavior of re-raising the first toolkit's exception instead of continuing to the next registered toolkit. As a result the documented toolkit precedence/fallback never happens for InChI conversion: per #2172, OpenEye refuses InChI generation for molecules with more than 1024 atoms (which RDKit supports), but the registry propagates OpenEye's error rather than falling back to RDKit.Fix
Pass
raise_exception_types=[]at these three call sites, matching the convention already used throughout the rest of theMoleculeAPI (e.g.assign_partial_charges, and as the test suite documents: "the Molecule API passes raise_exception_types=[] to ToolkitRegistry.call"). A higher-precedence toolkit that raises is now skipped in favor of the next one; if every registered toolkit fails,ToolkitRegistry.callstill raises a single aggregateValueErrordescribing each failure.Testing
Added
TestToolkitRegistry::test_inchi_methods_fall_back_through_toolkits, which registers a stub wrapper whose InChI methods always raise ahead ofRDKitToolkitWrapperand asserts thatto_inchi,to_inchikey, andfrom_inchiskip the failing wrapper and succeed via RDKit. The test fails onmain(the first toolkit'sValueErrorpropagates) and passes with this change; it runs offline with only RDKit (no OpenEye needed). All existing InChI tests continue to pass.Closes #2172.
Prepared with AI assistance under my direction; I reviewed and tested the change.