Fix initialise instances recursion - #116
Open
apdavison wants to merge 2 commits into
Open
Conversation
initialise_instances() recast each openMINDS library instance by round-tripping
through to_jsonld()/from_jsonld() with the default embed_linked_nodes=ALWAYS,
embedding every linked node. This recursed without bound over the cyclic
ParcellationEntity <-> ParcellationEntityVersion library graph (RecursionError,
and out-of-memory once a cycle guard was added on the openMINDS side).
Recast in two passes instead:
1. shallow recast with embed_linked_nodes=NEVER, so links serialise as
{"@id": ...} (KGProxy) and the graph is never traversed; collect the
recast objects in an id -> object lookup.
2. resolve each instance's links against that lookup, so cross-references
point at the actual recast fairgraph objects (links outside the set stay
KGProxy, resolvable from the KG later).
Also wrap the whole initialisation in set_error_handling(None) with a restore in
finally, so the intentionally-incomplete library instances emit no validation
output on 'import fairgraph'.
Adds tests for the resolved cross-references, a silent and non-crashing import,
and restoration of the default error handling.
The two-pass initialise_instances() relies on Node._resolve_links() tolerating links to ids outside the lookup, which is fixed in openMINDS 0.5.2.
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
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.
Fix unbounded recursion when recasting openMINDS library instances at import time
Background: what
initialise_instances()is foropenMINDS ships a set of library instances: pre-defined, controlled-vocabulary objects that live as class attributes on the openMINDS classes themselves, e.g.
openminds.v4.controlled_terms.Species.homo_sapiens, or the manysands.ParcellationEntity/sands.ParcellationEntityVersioninstances describing brain atlas regions. They are ordinary Python objects created at import time by theopenmindspackage.fairgraph subclasses each openMINDS class (
class Person(KGObject, OMPerson)) to add Knowledge Graph behaviour:.save(),.resolve(),.exists(), spaces, and so on. That means the library instances inherited from the openMINDS parent are of the wrong type for fairgraph users; they are plainopenmindsnodes with no KG behaviour.fairgraph.utility.initialise_instances()exists to fix that. It runs once, whenfairgraph.openmindsis imported, walks each fairgraph class, finds the library instances defined on its openMINDS parent, and recasts them to the fairgraphsubclass, rebinding them as attributes of the fairgraph class. So after
import fairgraph,fairgraph.openminds.controlled_terms.Species.homo_sapiensis aKGObjectyou can save to, or look up in, the KG.The recast was done by round-tripping through JSON-LD:
cls.from_jsonld(instance.to_jsonld()).The bug, and why it only appeared now
The round-trip recast worked for a long time, but a change in the upstream
openmindsmodule exposed a latent bug, which breaks fairgraph.Before. The openMINDS code generator emitted cross-references between library instances as raw
{"@id": ...}dicts. SoAccessibility.direct_virtual_open_access.payment_modelswas a plain dict, not aPaymentModelTypeobject. That was reported as openMINDS_Python issue #94:because the referenced instances were never objects, they never got added to a
Collection, and saving and reloading a collection raised aKeyError.Those dicts are also why fairgraph's recast worked.
to_jsonld()defaults toembed_linked_nodes=ALWAYS, meaning "serialise every linked node inline, recursively, rather than as an{"@id": ...}reference". When the links were not node objects in the first place, there was nothing forALWAYSto follow: the dicts were copied through verbatim, the recast never left the instance it started on, and it terminated immediately. The recursion hazard was in the code all along; the data never exercised it.After. openMINDS_Python PR #95 (merged 26 June 2026) addressed #94: the generator now resolves
@idreferences to the actual typed Python objects at generation time, falling back to raw dicts only where a reference cannot be resolved. To support mutually-referencing classes it alsorestructured generation, moving instances into separate
*_instances.pymodules that are imported after all the classes are defined.That turns the library instances into a real object graph, and that graph is cyclic.
sands.ParcellationEntityandsands.ParcellationEntityVersionnow point at each other (has_parents/has_versionsand friends) across many hundreds of atlas-region instances.embed_linked_nodes=ALWAYSfollows those links into the cycle, so a bareimport fairgraphraised aRecursionError. Adding a cycle guard on the openMINDS side stopped the infinite recursion, but the embedding then expanded combinatorially and the machine ran out of memory instead.The fix
initialise_instances()now recasts in two passes:embed_linked_nodes=LinkedNodeEmbedding.NEVER, so links come out as{"@id": ...}and the instance graph is never traversed. Deserialising gives a fairgraph object whose links areKGProxyplaceholders. Collect the results in anid -> recast objectlookup.Node._resolve_links(node_lookup)on each recast object, swapping eachKGProxyfor the actual recast fairgraph object where the id is in the lookup. (KGProxysubclassesopenminds.base.Link, so the upstream resolver handles it directly.) Links pointing outside the library set are left asKGProxyand remain resolvable from the KG later, exactly as for any other fetched object.The whole thing is two flat passes over a finite set of objects, which means there is no graph traversal, no recursion, no cycles to fall into.
Additionally, the initialisation is now wrapped in
fairgraph.openminds.set_error_handling(None)with a restore in afinallyblock, soimport fairgraphis again silent, without validation noise coming from the recasting process. Thefinallyrestores the default"log"handling, so validation behaviour for user code is unchanged.Dependency bump
This PR requires
openminds>=0.5.2, for two reasons: it needs the PR #95 generation changes described above, and the second pass relies onNode._resolve_links()tolerating links whose id is not in the lookup (leaving theLinkin place rather than raisingKeyError).Note that openMINDS 0.5.2 (or 0.6.0) has not been released yet, The changes this PR depends on are available in the openMINDS
mainbranch, but not in a release. This is also why the CI tests are currently failing. This PR should therefore be held until openMINDS 0.5.2 or 0.6.0 is out; merging sooner would leave fairgraph declaring a dependency that cannot be installed. Ensure the CI tests are re-run and passing before merging.Conversely, once a new
openmindsversion is released, a new fairgraph release including this PR needs to follow as soon as possible.