Replace graph store with Oxigraph + OntoEnv 0.6 - #396
Open
gtfierro wants to merge 18 commits into
Open
Conversation
Wire Library.load(ontology_graph=...) through OntologyEnvironment so owl:imports are fetched by OntoEnv and stored in BuildingMOTIF's Oxigraph graph store rather than resolved manually. ShapeCollection resolve_imports and infer_templates now delegate to OntoEnv as well, removing the old recursive _resolve_imports helper.
pytest_generate_tests was calling setup_building_motif_brick/s223 at collection time, causing Brick.ttl, 223p.ttl, and all QUDT dependencies to be loaded into an in-memory SQLite DB just for --collect-only. Replace with cheap YAML-key + rdflib-local-parse extraction at collection time (~0.5s), and move the full Library.load into session-scoped fixtures that run once during test execution. Collection time drops from minutes to under 1s.
…ntoenv - Add init_from_store=True to OntologyEnvironment so libraries already persisted in Oxigraph are visible to ontoenv import resolution on restart - Remove xsd:string-stripping branch from _decode_term; RDF 1.1 treats plain strings and xsd:string as equal, and the strip made triples() and query() return incompatible objects for the same stored value - Fix false-positive OntologyImportsNotFound in resolve_imports fallback path: track whether the fallback was used and check missing_imports against self.graph instead of graph_name to avoid index-based false positives - Add warning log to resolve_imports except-branch to match infer_templates - Wrap Oxigraph graph write in try/except in _load_from_ontology; on failure, clears any partial write and rolls back the SQL session for consistency - Forward infer_templates and run_shacl_inference through _load_from_directory to _load_shapes_from_directory (flags were silently ignored before) - Add try/finally to BuildingMOTIF.close() so all resources are released even if an earlier close step raises - Extract OntologyEnvironment.ensure_and_get_closure() helper to remove the duplicated ensure-registered+closure_copy pattern from resolve_imports and infer_templates - Update CLI docs to document --graph-store-path on load and serve subcommands - Update developer docs Python minimum version (3.10 -> 3.11)
Replace in-place graph mutation with copy-on-write. Model.replace_graph and ShapeCollection.replace_graph write new contents into a fresh graph_id and flip the SQL pointer, so a failure or session rollback leaves the previous graph intact instead of leaving a half-written graph. This removes the hand-rolled cleanup in Library.load and fixes the remove-then-add hazard in the model PUT endpoint. Add BuildingMOTIF.collect_graph_garbage to reclaim orphaned named graphs left by replacement and by row deletion. Only UUID-keyed graphs (models, shape collections, template bodies) are considered, so OntoEnv's IRI-keyed ontology graphs are excluded by construction. Runs best-effort on close(). Document the dual-store backend design (graph_id pointer, consistency model, copy-on-write, garbage collection) in a new explanations page.
Add GraphConnection.load_file_into_graph, which parses RDF files directly into a named graph using Oxigraph's native (Rust) loader instead of rdflib's per-triple Python path. On Brick.ttl (~54k triples) this is ~11x faster (0.80s -> 0.07s) and produces an identical stored graph. The library directory loader now uses it. Native loading is used only for trusted on-disk RDF (the native parser requires valid IRIs, unlike in-memory graphs that may carry generated template parameters), does not propagate file prefixes (callers re-bind the standard prefixes), and falls back to rdflib.parse if a file cannot be loaded natively, so behavior is never worse than before.
- test_database_persistence: snapshot store-backed graphs before bm.close() (Oxigraph deletes _store on close, making live views inaccessible); call BuildingMOTIF.clean() before reopening so the singleton is fresh; filter bare UUID graph identifiers out of BuildingMOTIFGraphStore.graph_ids() so OntoEnv's init_from_store does not choke on non-IRI names - dataclasses: add field(compare=False) to _bm and graph fields on Library, Template, Model, and ShapeCollection so __eq__ compares identity (id/name) rather than session pointers or graph object identity - notebooks/Session-example.ipynb: replace dead SQL table queries (kb_625d302a74_type_statements) with RDFLib graph API; graph data is now in Oxigraph, not the SQLite relational store - notebooks/223P-Validation.ipynb: fix relative path to 223p.ttl (notebooks/ runs as cwd, so ../libraries/... is correct)
In ontoenv 0.6.x, copy_closure and iter_closure_triples read from the native Rust store and bypass the custom graph_store adapter. When BuildingMOTIF's BuildingMOTIFGraphStore is in use, this returns empty graphs/iterators even though the ontology data is correctly stored. get_closure returns a ClosureGraphView that correctly routes through the adapter. Also bumps the BACnet integration test Dockerfile from python:3.10 to 3.11, matching the pyproject.toml minimum.
Collaborator
Author
|
@TShapinsky @MatthewSteen I'm in the middle of making some major changes to buildingmotif to use new tooling I have been developing. This will help with performance issues, which in my view have been a significant challenge to using bmotif. |
_load_shapes_from_directory loaded every file in a library directory straight into the shape collection's graph via Oxigraph's native loader, but never told OntoEnv these ontologies exist. Any owl:imports referencing them - e.g. mediumOffice_constraints.ttl importing urn:ashrae/g36, which guideline36.ttl declares - couldn't be resolved by name, so OntoEnv fell back to treating the import as a relative file path and failed with "No such file or directory". Register each file with the ontology environment alongside the existing bulk graph load. Files without their own owl:Ontology header (guideline36's per-equipment fragments) just get a harmless synthetic name; files that do declare an identity (Brick's imports/*.ttl, which import each other) become resolvable the same way single-file Library.load(ontology_graph=...) already registers them. Verified: without this change, missing_imports() on a graph importing urn:ashrae/g36 reports it as missing after loading the guideline36 directory; with it, the import resolves.
Bumps the pin from 0.6.0-a5. None of a8's breaking changes touch the API BuildingMOTIF uses: get_graph() becoming a read-only view does not matter because we only ever call copy_graph, we call neither snapshot_as_dataset nor as_dataset, and we import none of the removed top-level helpers. poetry lock resolved with no other dependency movement. The bump also makes a fix worth taking. Two of the three closure call sites read `_, closure_names = closure_copy(...)` -- materializing the entire imports closure into an rdflib graph, one term at a time across the FFI boundary, and then discarding it to keep a list of names. Both now call the new OntologyEnvironment.closure_names, which wraps ontoenv's list_closure. Measured on the Brick 1.4 closure (15 graphs, ~155k triples), three reps: list_closure 0.000s 0.000s 0.000s copy_closure 4.271s 3.879s 5.322s (materializes every call) get_closure 8.846s 0.000s 0.000s (eager permutation indexes, then free) All three report the same 15 names. So this is ~4-5s saved per ontology load that follows imports, and library loading does it for every ontology. get_closure -- a8's read-only zero-copy view -- was the obvious-looking fix and is the wrong one here: it builds all four permutation indexes eagerly at bind time, which pays off for repeated queries against a closure but is pure overhead for a one-shot name lookup. closure_copy remains in use at ontology_environment.py:133, whose caller gets a graph it may mutate; a ViewGraph would raise ValueError there. Note for anyone reading ontoenv's changelog against this pin: the `get_closure_view` described under [Unreleased] does not exist in a8. That section is ahead of the published alphas, and a8 spells the read-only view `get_closure`.
…tore a8 deprecates the `init_from_store` flag: "use OntoEnv.adopt(...) for first-time adoption". BuildingMOTIF passes it on every construction that has a graph_connection, which is all of them, so the unit suite went from 58 warnings to 527 -- one per BuildingMOTIF built. The replacement is not a single call, because the two cases genuinely differ: - persistent (a cache path): OntoEnv.connect(path, graph_store=...). connect is by definition "create it, or reuse the saved index", which is what the old create_or_use_cached meant, and it subsumes init_from_store too -- sync="auto" reads graph contents only when it first meets a populated store or when the store reports a change. - temporary (no path): there is no saved index to reuse, so a populated store still has to be scanned outright, via refresh_from_store(full=True). That is what init_from_store was doing at construction time. `adopt` -- the method the warning names -- is the deliberate first-time scan of a pre-populated store and always persists an index, so it fits neither branch: the temporary case has nowhere to persist to, and the persistent case wants reuse rather than re-adoption on every open. No behaviour change intended; the unit suite is the check, since `path=None` with a graph store is the branch every test exercises.
Pin 0.6.0-a8 -> 0.6.0-a9. a9's breaking changes do not reach BuildingMOTIF: get_closure/get_union now return a read-only ViewGraph, but we call copy_closure and copy_graph; snapshot_as_dataset/to_rdflib_dataset are deprecated in favour of get_dataset/copy_dataset, and we call none of them; update(all=) is superseded by update(force=), and we never call update. Membership now goes through the container protocol. `ensure_and_get_closure` tested `graph_name not in self.ontology_names()`, which materializes every ontology name to answer one question; `knows()` wraps `ontology in self.env`. It is also broader in a useful way -- `in` resolves aliases and source URLs as well as canonical names, so an ontology known under another spelling is no longer needlessly re-added. Verified equivalent on the canonical-name case: sorted(get_ontology_names()) == sorted(list(env)). Re-measured the closure paths on a9, since the docstring quoted a8 numbers and one of them changed shape (Brick 1.4 closure, 15 graphs, ~155k triples, x3): list_closure 0.000s 0.000s 0.000s copy_closure 3.972s 3.530s 3.623s get_closure 2.053s 2.077s 2.059s On a8 get_closure read 8.846s / 0.000s / 0.000s -- one eager index build then cached. On a9 it is a flat ~2.05s: cheaper on first use, no longer free on repeat. list_closure remains the right call for names either way. Also recorded, because it is load-bearing for `ensure_and_get_closure`'s return type and is easy to assume otherwise: a ViewGraph is **not** an rdflib.Graph subclass (isinstance check: False, type ViewGraph), so it cannot be returned through a signature promising rdflib.Graph. The persistent-cache defect is NOT fixed by a9: with a custom graph_store, .ontoenv/catalog.pending still survives a clean close and the next open raises CatalogRecoveryError. Reproduced on a8 and a9 alike.
A persistent ontology cache could be opened exactly once. The second open
raised CatalogRecoveryError and there is no way back: ontoenv exposes no
recovery API, so the cache was permanently unusable.
Root cause, traced by wrapping each OntoEnv method and watching the marker
file: ontoenv writes .ontoenv/catalog.pending when a batched mutation begins
and removes it only when the batch finishes cleanly (BatchScope::run in
lib/src/api.rs; its Drop impl does not remove it either). An unresolvable
owl:imports makes the batch end in error, so the marker survives.
That is the normal case here, not an edge case. Loading Brick calls
import_dependencies, which raises ValueError("Failed to resolve graph for URI")
for each of eight imports Brick declares and that do not resolve offline --
brickschema 1.3, qudt unit and quantitykind, ashrae bacnet/2020, Brick/ref,
rec/brickpatches, rec/recimports, datashapes dash. Several 404 even online.
BuildingMOTIF tolerates missing imports and swallows those errors, so the load
"succeeds" while the catalog is left marked. An ontology declaring no imports
leaves no marker, which is how this was isolated.
So `connect` now clears a marker it finds and retries once, with a warning
naming the file. This is a workaround for an upstream defect and is marked as
such: it trades away the marker's value after a genuine crash mid-write, which
is the reason it warns rather than doing it silently, and only retries once.
Verified: load Brick into a persistent cache, close, reopen (warns, succeeds,
ontology intact), close, open a third time cleanly.
The principled fix belongs upstream -- a tolerable non-strict import failure
should not leave an interrupted-mutation marker, since non-strict mode is
documented as best-effort. Failing that, ontoenv should expose a recovery call
so consumers do not have to delete files it owns.
Pin 0.6.0-a9 -> ^0.6.0, the first stable 0.6 release. A caret range rather than the exact pin the alphas needed: 0.6.0 is a published release with a migration guide, so patch fixes are wanted, not feared. 0.6.0 closes the two upstream defects this branch had been working around, and both workarounds go with it. Deleting .ontoenv/catalog.pending by hand is gone. That existed because a merely unresolvable owl:imports left the interrupted-mutation marker behind, which made a single Library.load(ontology_graph=Brick) poison a persistent cache permanently -- routine here, not exceptional, since Brick declares imports that 404 even online. In 0.6.0 a tolerated missing import commits cleanly and leaves no marker, so reaching that handler now means an actually interrupted write. Verified: load Brick into a persistent cache, close, and the marker is absent; opens #2 and #3 both succeed with the ontology intact. _connect_recovering therefore stops removing a file ontoenv owns -- which 0.6's docs now explicitly warn against -- and calls OntoEnv.recover(path, graph_store=...) instead, the supported rebuild. It is taken automatically rather than surfaced, because the documented answer is unreachable for our callers: recovering a custom store requires passing that store, and ours does not exist until a BuildingMOTIF has been constructed, which is the thing that just failed. Recovery rescans every stored graph, so it warns; ontoenv only clears the marker once the replacement index is published, so a failed recovery still raises. Shape-collection dependency collection stops swallowing every exception. 0.6.0 raises UnresolvedImportError (a LookupError) for every unresolved owl:imports target -- confirmed 6 of 6 on the Brick closure, where earlier builds left some as bare ValueError -- so the expected case can be caught precisely and a storage error or malformed IRI now propagates instead of being logged as a missing dependency and skipped. The exception is re-exported from ontology_environment so ontoenv stays behind that seam. No API of ours changed. get_closure/get_union still return a ViewGraph that does not subclass rdflib.Graph, so the copy in ensure_and_get_closure stays; closure_names' benchmark is re-measured on 0.6.0 (list_closure free, copy_closure ~4.2s, get_closure ~2.4s over Brick's 15-graph, 155k-triple closure) and its conclusion is unchanged. No new deprecation warnings: the create_or_use_cached and init_from_store transitions were done on a8.
gtfierro
changed the title
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.
Swaps out the old
rdflib-sqlalchemygraph store, which kept triples inside the SQL database and was slow because every SPARQL query became a pile of joins. Triples now live in Oxigraph (viaoxrdflib) with real triple indexes, and metadata stays in SQLAlchemy. The two stores are linked by an opaquegraph_idpointer per row. There's a new explanations page (docs/explanations/storage-architecture.md) that walks through the whole design if you want the details.rdflib-sqlalchemywithoxrdflib, meaning faster SPARQL queriesGraphConnection.load_file_into_graph) — ~11x faster on Brick.ttl (0.80s → 0.07s), falls back toGraph.parseif native loading can't handle a file, so it's never worse than before. Currently only used for trusted on-disk RDF.owl:importsresolution.Library.load,ShapeCollection.resolve_imports, andinfer_templatesall delegate to it instead of the old hand-rolled recursive resolver, and imported ontologies share the same Oxigraph storeModel.replace_graph/ShapeCollection.replace_graphwrite into a freshgraph_idand flip the SQL pointer last, so a crash orsession.rollback()leaves the old graph intact instead of a half-written one. Replaces the manual cleanup inLibrary.loadand the remove-then-add stuff in thePUTendpoint. Also opens the path for doing versioned models down the roadBuildingMOTIF.collect_graph_garbagereclaims orphaned graphs left behind by COW and deletes. Runs onclose().--graph-store-pathto theload/serveCLI subcommands, and moved ontology loading out of pytest collection into session fixtures so--collect-onlygoes from minutes to under 1s.One major change: the graph store is now a separate on-disk artifact from the SQLite metadata DB, and existing
rdflib-sqlalchemydatabases aren't migrated by this PR.OntoEnv 0.6
The OntoEnv pin moved
0.6.0-a5→0.6.0over the last five commits, following its 0.5 → 0.6 migration. All of it is confined tobuildingmotif/ontology_environment.py, the one place BuildingMOTIF talks to OntoEnv, apart from one exception type inshape_collection.py.create_or_use_cached=Trueandinit_from_store=Trueare both deprecated in 0.6. Persistent environments now useOntoEnv.connect(path, graph_store=...); temporary ones useOntoEnv(graph_store=..., temporary=True)plus an explicitrefresh_from_store(full=True), since there's no saved index for a temporary env to reuse. Notadopt, despite it being the method the deprecation names —adoptalways persists an index, which fits neither case. This also removed ~470 deprecation warnings per test run (one perBuildingMOTIF())._, names = copy_closure(...)— building the entire imports closure across the FFI boundary just to keep a list of names. NowOntologyEnvironment.closure_names, wrappinglist_closure. On the Brick 1.4 closure (15 graphs, ~155k triples):list_closureis free wherecopy_closurecosts ~4s andget_closure~2.4s, all three returning the same 15 names.ontology_cache_pathworks for the first time. It was never usable: the old code passedgraph_storeandcreate_or_use_cached=Truetogether, which OntoEnv rejects outright. Only the temporary path is exercised by the suite (ontology_cache_pathdefaults toNone), which is why this stayed invisible..ontoenv/catalog.pendingmarks an interrupted write, and finding it at open time raisesCatalogRecoveryError._connect_recoveringcallsOntoEnv.recover(path, graph_store=...), logging a warning first because recovery rescans every stored graph. It's automatic rather than surfaced to the caller because the documented fix isn't reachable from outside: recovering a custom store means passing that store, and BuildingMOTIF's doesn't exist until aBuildingMOTIFhas been constructed — which is what just failed. OntoEnv clears the marker only after publishing the replacement index, so a failed recovery still raises. Verified by planting a marker on a populated cache: warns, rebuilds, ontologies intact, marker cleared, next open clean.UnresolvedImportError(aLookupError) for every unresolvedowl:importstarget, soinfer_templatescatches that instead of everyExceptionwhen collecting per-dependency graphs — a storage error or malformed IRI now propagates rather than being logged as a missing dependency and skipped. Re-exported fromontology_environmentso callers don't importontoenvdirectly.ontology in envreplaces building the full name list to test one membership, and is broader: it resolves aliases and source URLs as well as canonical names, so an ontology known under another spelling isn't needlessly re-added.get_closure/get_unionreturn a read-onlyViewGraphin 0.6 that deliberately doesn't subclassrdflib.Graph, so the paths that hand a mutable graph back to callers still usecopy_*. Nothing in BuildingMOTIF's own API changed.The pin is a range (
^0.6.0) rather than the exact pin the alphas needed. Unit suite is unchanged across the whole migration: 535 passed, 1 skipped.