Skip to content

Fix initialise instances recursion - #116

Open
apdavison wants to merge 2 commits into
HumanBrainProject:masterfrom
apdavison:fix-initialise-instances-recursion
Open

Fix initialise instances recursion#116
apdavison wants to merge 2 commits into
HumanBrainProject:masterfrom
apdavison:fix-initialise-instances-recursion

Conversation

@apdavison

@apdavison apdavison commented Aug 7, 2026

Copy link
Copy Markdown
Member

Fix unbounded recursion when recasting openMINDS library instances at import time

Background: what initialise_instances() is for

openMINDS 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 many
sands.ParcellationEntity / sands.ParcellationEntityVersion instances describing brain atlas regions. They are ordinary Python objects created at import time by the openminds package.

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 plain openminds nodes with no KG behaviour.

fairgraph.utility.initialise_instances() exists to fix that. It runs once, when fairgraph.openminds is imported, walks each fairgraph class, finds the library instances defined on its openMINDS parent, and recasts them to the fairgraph
subclass, rebinding them as attributes of the fairgraph class. So after import fairgraph, fairgraph.openminds.controlled_terms.Species.homo_sapiens is a KGObject you 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 openminds module exposed a latent bug, which breaks fairgraph.

Before. The openMINDS code generator emitted cross-references between library instances as raw {"@id": ...} dicts. So Accessibility.direct_virtual_open_access.payment_models was a plain dict, not a PaymentModelType object. 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 a KeyError.

Those dicts are also why fairgraph's recast worked. to_jsonld() defaults to embed_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 for ALWAYS to 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 @id references 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 also
restructured generation, moving instances into separate *_instances.py modules 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.ParcellationEntity and sands.ParcellationEntityVersion now point at each other (has_parents / has_versions and friends) across many hundreds of atlas-region instances. embed_linked_nodes=ALWAYS follows those links into the cycle, so a bare
import fairgraph raised a RecursionError. 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:

  1. Shallow recast. Serialise each instance with 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 are KGProxy placeholders. Collect the results in an id -> recast object lookup.
  2. Link resolution. Call Node._resolve_links(node_lookup) on each recast object, swapping each KGProxy for the actual recast fairgraph object where the id is in the lookup. (KGProxy subclasses openminds.base.Link, so the upstream resolver handles it directly.) Links pointing outside the library set are left as KGProxy and 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 infairgraph.openminds.set_error_handling(None) with a restore in a finally block, so import fairgraph is again silent, without validation noise coming from the recasting process. The finally restores 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 on Node._resolve_links() tolerating links whose id is not in the lookup (leaving the Link in place rather than raising KeyError).

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 main branch, 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 openminds version is released, a new fairgraph release including this PR needs to follow as soon as possible.

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.
@apdavison apdavison added the bug Something isn't working label Aug 7, 2026
@apdavison apdavison moved this from Todo to In Progress in fairgraph development Aug 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

1 participant