Repository navigation
fix: resolve instance types from the type each instance declares - #184
Merged
Merged
Conversation
The openMINDS type of an instance was taken from the name of the folder the instance is stored in. Folder names are pluralized type names, so a hand-maintained plural to singular map was needed to recover the type. Upstream renames those folders whenever a type is renamed, which the map cannot follow: it still mapped brainAtlases and commonCoordinateSpaces, renamed to anatomicalAtlases and commonCoordinateFrameworks in v5.0, and had no entry for accessibilities at all. Loading the library warned five times and left 115 instances of five types untyped. Every instance document declares its own type, which no rename can invalidate, so the type is now read from "@type" and the module from the type itself. All instances in a folder share one type, so one document per folder is enough to type the whole library. Nearly every instance IRI names its type in the singular, which the Types enumeration matches directly. The few that name it in the plural are resolved through the instance library, which indexes the IRI segments the instance documents use. openMINDS publishes no plural to singular mapping, and this carries none. Selecting another model version now rebuilds the library, since instances are typed against the version that was on the path when the table was built. An instance whose type the active model does not declare is listed without a type and reported once, rather than failing the load. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Instances are typed against the model version that was on the search path when the library was read, so selecting another version leaves the table naming types the active model may not declare. selectModelVersion now tells the instance library which version it selected, and the library rebuilds itself in place so that code holding a reference to it sees the new table. Only a library that already exists is rebuilt: creating one here would download the instance repository as a side effect of selecting a version, which openminds.startup does on every session. The version is passed in rather than read back from the search path, because that lookup caches its result for a second and would still name the previous version. It is formatted the way openminds.version reports it, so the check in getSingleton recognizes the library as current instead of rebuilding it a second time. That check stays as the safety net for a version change that does not go through selectModelVersion, since the active version is derived from the search path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fromAtType checked the namespace of an @type by calling openminds.constant.BaseIRI for both known namespaces on every call. That function declares its argument as a VersionNumber validated by mustBeValidModelVersion, so each call listed the installed model versions from disk and built the version objects to compare against, for arguments that are the literals "v1" and "v4". Resolving 191 types spent 2.93 s there, against 0.05 s to read the documents the types came from. The base IRI a version maps to is fixed and does not depend on which model version is active, so both are now resolved once. The same 191 types resolve in 0.025 s, and reading the instance library drops from 4.81 s to 1.69 s. fromAtType runs once per node of every document read, so deserialization paid the same cost per node. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The instance library publishes one set of instances per model version, but the version to read was a public property fixed at "latest" that nothing ever set. Selecting model version v3.0 therefore read the latest instances, and reading one version's instances against another version's types is exactly what leaves instances untyped: 3132 of them, against none when the versions match. LibraryVersion now follows the model version and is read-only. Selecting a model version picks the matching instances, and the two can no longer drift apart. The library publishes no instances for model versions 1 and 2, which predate the type names it is written against, and no other version of the library can stand in for them. That is now reported, and the library is left with tables that keep their columns so that listing instances answers with none rather than erroring. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Test Results (R2022a)849 tests 845 ✅ 3m 45s ⏱️ Results for commit f21575b. ♻️ This comment has been updated with latest results. |
Contributor
Test Results (R2026a)849 tests 847 ✅ 3m 58s ⏱️ Results for commit f21575b. ♻️ This comment has been updated with latest results. |
…library Selecting a model version rebuilds the instance library, and listing the instances of a version that holds none raised an error. That error propagated out of selectModelVersion, so a model version could no longer be selected at all when the library was incomplete, which is how the library reaches a CI runner that has just downloaded it. Tests that only switch model versions failed on it. Selecting a model version is a change to the search path. The instance library is a separate resource that may be absent, incomplete, or not published for the version, so all of those now leave the library with empty tables that keep their columns, and report why, rather than stopping the version from being selected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #184 +/- ##
==========================================
+ Coverage 79.56% 80.09% +0.53%
==========================================
Files 422 423 +1
Lines 4252 4381 +129
==========================================
+ Hits 3383 3509 +126
- Misses 869 872 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The library location is built under userpath, and userpath is empty when its default folder does not exist, as on a fresh CI runner. The location is then relative, and stops naming the library as soon as anything changes the working directory, which the test suite does through WorkingFolderFixture. That went unnoticed while the library was read once, at construction, before any test had changed folder. Reading it again on a model version change moved the read after that point: the versions detected at construction still listed the version, while its files no longer resolved, so the library reported the version as holding no instances. The location is now resolved against the working directory when it is set, so it keeps naming the same folder however the working directory moves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
MATLAB's default user folder does not exist on a runner, so userpath is empty and the folders the toolbox keeps under it come out relative. The live script workflow already runs ensureUserpath for this reason; the test workflow did not, and ran against an instance library placed relative to the working directory. It runs before anything else is put on the path, because those folders are read from Constant properties and MATLAB evaluates those once per session: reading one before userpath is set freezes the relative value for the rest of the run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The toolbox keeps the schemas and instance library it downloads under MATLAB's user folder, which does not exist on a runner. Two copies of the remedy had grown apart: a script under tools that the live script workflow ran, pointing userpath at RUNNER_TEMP, and a block inside setup.m pointing it at the working directory. The block in setup.m never had an effect: openminds.startup runs before it and reads a path built under the user folder, and those are constant properties, which MATLAB evaluates once when the class is first loaded. Pointing userpath at the working directory was also what put a library of thousands of files inside the checked out repository. Both are replaced by openminds.internal.setup.ensureUserpath, which ships with the toolbox and prefers the folder a GitHub Actions runner names for this. setup.m does not call it, because setup.m is only run from a clone, never from an installed toolbox. Nothing inside the toolbox can call it either: the validation of an argument to openminds.startup already reads one of those paths, so the call has to come before the toolbox is entered, which is where the workflows now make it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The library resolves its location against the working directory when it is set, but getSingleton compared the location a caller asked for as given. One folder named two ways then read as two libraries, so a relative location, which is what an empty userpath produces, deleted and rebuilt the library on every call and invalidated every handle held to it. Both now resolve the location the same way, through one rule the class owns rather than a rule the setter kept to itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The folders the toolbox downloads into sit under MATLAB's user folder, and were declared as constant properties. MATLAB evaluates those once, when the class is first loaded, which is before there is necessarily a user folder to build them under: a runner whose user folder does not exist leaves userpath empty and every folder built under it relative. Validating an argument to openminds.startup already loads this class, so nothing inside the toolbox could get ahead of it, and the remedy had to be run from outside before MATLAB entered the toolbox at all. They are resolved when they are first asked for instead, which is always late enough because it is the moment the answer is needed, and gives MATLAB a user folder if it has none. Resolving them on every call would answer differently once userpath changed and move the instance library out from under whoever was reading it, so the answer is kept for the session. It is also kept because fullfile costs enough to notice on a lookup that runs once per controlled instance read: resolved per call the instance folder took 1012 us, against 8.5 us kept and 7.1 us as a constant. GeneratedFolder stays constant. It sits inside the toolbox, which cannot move while MATLAB is running, and it is read on the paths that run most. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…r folder MATLAB having no user folder left the toolbox choosing where to write tens of megabytes, and it chose the working directory. That is as often as not a repository or a project folder, and the choice was recorded as the user folder for good, naming a directory that was only ever where MATLAB happened to be standing. The temporary folder is used instead: also not a folder anyone asked for, but a worse place to lose files is better than a worse place to find them. The choice is now said out loud, as a warning with an identifier rather than a line of printed text, naming the folder and how to pick another. It is said once, because the folders built on it are resolved once a session. The folder is read back from MATLAB rather than kept as given: tempdir ends in a separator and MATLAB normalizes what it stores, so the same folder was named two ways on the first call and every call after it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The toolbox picked a user folder for MATLAB by asking whether it was running on a GitHub Actions runner. Which runner we are on is not the toolbox's business: it shipped code that inspects an environment variable a user could set by accident, and behaviour that cannot be exercised from inside the toolbox. The toolbox now picks the temporary folder when MATLAB has no user folder, and nothing else. A task under tools knows about the runner and sets the folder the runner offers, before the toolbox picks its own; anywhere else it does nothing and leaves the choice to the toolbox. testToolbox and the live script workflow run that task, and the calls that reached into the toolbox from the workflows to do the same are gone. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ectly Three things a review of the branch found wrong in what the instance library says and when it says it. The warning for an instance type the model does not declare told the user to set the library version to match the model version. The library version follows the model version and cannot be set, so that advice could not be taken; and it named the model version by reading it back from the search path, which is cached for a second and is the reason the version is passed in explicitly everywhere else. It now names the versions it was given, and says what a mismatch between coupled versions means: the library is ahead of the model, or the model classes in memory belong to a version selected earlier in the session. A document with no readable "@type", from a file that could not be opened or one that was truncated, was skipped without a word and its folder left untyped. Those are now collected and reported like an undeclared type. The versions were recorded before the read that could fail, so a read that failed part way left the version saying the table was current while the table was the previous version's. Everything the read produces is now assigned together, last, with the root folder and the versions passed down to what needs them rather than read from an object not yet updated. That also retires a dependent property that had no answer while the library version was missing. Also from the review: the setget mixin was inherited for a call that no longer exists; a docstring described the folders under the user folder as constant properties that had to be resolved ahead of time, which they no longer are; the two header readers signalled "not found" differently; and the test of whether a path is absolute duplicated the implementation under test rather than checking it independently. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Resolving a relative path against the working directory is not something the instance library knows how to do better than anything else, and as a private static method of it nothing else could use it. It is now openminds.internal.utility.resolveAbsolutePath, beside the other path helpers. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The warning for an instance whose type could not be resolved said that the model version did not declare the type. That is not what was checked, and it is false in the case that raises the warning most: the classes in memory belong to a version selected earlier in the session, and nothing has let MATLAB read the selected version's classes yet. The types named are then exactly those the two versions do not share, all of which the selected version declares. The warning now says what was checked, the model classes loaded in this session, and gives the two things that can mean and what to do about the second, rather than telling the user to restart MATLAB. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
On a runner whose default user folder does not exist, which is the case this task exists for, MATLAB warns that it cannot locate the personal folder while the user folder is being set. The warning is switched off around setting it and restored afterwards. The task also returns nothing when nothing is asked for, so that calling it as a statement, as testToolbox does, does not print ans. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The comments added on this branch described mechanisms indirectly, and some described the wrong problem: they said MATLAB has no user folder, when the problem is that userpath is empty on Linux when $HOME/Documents does not exist, as it is on CI runners. Every comment added on the branch now states the mechanism first and the consequence second. The Paths class header gives the two reasons the user folder paths are cached static methods rather than Constant properties. Both ensureUserpath functions say what they set, when, and what they leave alone. The instance library comments name the one second cache in openminds.version instead of calling it brief, and the two causes an unresolved instance type can have. The test comments say what is asserted and why. The warning raised when userpath is unusable now says so, instead of saying MATLAB has no user folder. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A library folder that exists but holds no version folder is an incomplete download that the recorded commit did not notice. Nothing warned about it, so every controlled instance lookup silently found nothing. resolveLibraryVersion now warns with OPENMINDS:InstanceLibrary:NoVersionsFound in that case. A folder that does not exist stays silent here, because the failed download has already been reported. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
- Move createInstanceTable, resolveFolderInfo and createIRISegmentIndex out of the class into local functions. None of them read the object, and the private method block they sat in had no other members. - Name the IRI segment index columns once, in iriSegmentIndexVariableNames, instead of repeating the literal. - Spell out the staleness test in getSingleton as one named condition. - Reword the comments in ensureUserpath, testToolbox and resolveFolderInfo that a reader had to reread. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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.
Background
All controlled instances (openMINDS_instances) are stored in a folder hierarchy organized by types. In this toolbox, the type of an instance was taken from the name of the folder the instance is stored in. However, folder names are pluralized type names, so a hand-maintained plural-to-singular map was needed to recover the type. Upstream (openMINDS_instances) renames those folders whenever a type is renamed, so a map must be kept up to date.
Problem
Currently this map is outdated, it still mapped
brainAtlasesandcommonCoordinateSpaces, renamed toanatomicalAtlasesandcommonCoordinateFrameworksin v5.0, and had no entry foraccessibilitiesat all. Loading theInstanceLibrarywarned five times and left 115 instances of five types untyped.parseInstanceIRIfailed on the same instances, because it read the type from the second-to-last IRI segment.Solution
Every instance document declares its own type (via the
@typefield), which no rename can invalidate. The type is now read from@typeand the module from the type itself. All instances in a folder share one type, so one document per folder types the whole library. This removesgetPluralToSingularTypeMapandSandsInstanceFolders, and carries no mapping in their place.Of the 118 IRI segments in the library, 115 name their type in the singular and the
Typesenumeration matches them directly. The three that are plural —accessibilities,contentTypes,licenses— are resolved through the instance library, which indexes the segments the instance documents use.Related
Versions
Two version problems surfaced from making types resolve against the model. Instances are typed against the model version that was on the path when the table was read, so
selectModelVersionnow rebuilds the library in place, and only one that already exists, since creating one would download the instance repository as a side effect of selecting a version. And the library version, a public property fixed at"latest"that nothing ever set, now follows the model version and is read-only: reading one version's instances against another version's types left 3132 instances untyped, against none when they match. Model versions 1 and 2 predate the type names the library is written against, and no version of the library can stand in for them, so that is reported rather than left looking like an empty library.Selecting a model version no longer fails when the library is absent or incomplete for that version. The library is a separate, downloaded resource, and not being able to read it is not a reason for a change to the search path to fail; it leaves empty tables that keep their columns, and says why.
Performance
Resolving a type turned out to cost 15.5 ms.
fromAtTypelooked up the base IRI of both known namespaces on every call, and each lookup validated its literal argument by listing the installed model versions from disk. Both are now resolved once. Reading the library drops from 4.81 s to 1.69 s, below the 2.46 s the folder-name version took, and deserialization pays the same cost per node of every document read.fromAtType× 191Where the library lives
Re-reading the library on a version change exposed that its location could be relative. The location is built under
userpath, which is empty wherever MATLAB's default user folder does not exist, including every GitHub Actions runner. A relative location stops naming the library as soon as anything changes the working directory, which the test suite does throughWorkingFolderFixture; the versions detected at construction still listed a version whose files no longer resolved. The location is now resolved to an absolute path when it is set, andgetSingletonresolves the location a caller asks for the same way, so one folder named two ways is no longer two libraries that rebuild each other on every call.The folders under the user folder were constant properties. MATLAB evaluates those once, when the class is first loaded, and validating an argument to
openminds.startupalready loads it, so nothing inside the toolbox could set a user folder ahead of them. They now resolve when first asked for and are kept for the session, so they neither freeze too early nor move the library out from under a reader whenuserpathchanges later. When MATLAB has no user folder the toolbox uses the temporary folder, not the working directory, and warns once, naming the folder and how to choose another. A runner has a better folder to offer; knowing which runner we are on is not the toolbox's business, so a task undertoolssets that before the tests run, and nothing shipped inspects CI environment variables.Tests
InstanceLibraryTestis new.testEveryInstanceIsTypedis the one that matters: it fails on any untyped row, which is what the v5.0 rename produced and what nothing caught. It also covers the version coupling, the rebuild on a version change, the report for a model version without instances, and the location surviving a change of working directory.PathsTestis new and covers the resolved-once folders.ValidatorsTestandEnumerationTestgain the plural-segment cases and a guard against the base IRIs being resolved per call again.Notes
ModelVersionFixturedocuments that class definitions are not reloaded while instances of them are held in memory. A test asserting that matching the versions leaves nothing untyped therefore passes alone and fails in a full suite, so the test checks that the selected version's instances are the ones read, and leaves the reloading to MATLAB.Model versions v1.0 and v2.0 cannot load
openminds.enum.Typesat all: their generated enumerations name classes that were never generated,AtlasTerminologyfor v1.0 andURLfor v2.0. Untouched here. Whether those versions should ship at all is an open question, so no issue is filed for the enumerations themselves.Three pre-existing bugs found on the way are filed separately and not fixed here: #185 (
getSingletonrejects a missing library folder before the class can download it), #186 (getControlledInstancebuilds folder names by pluralizing, and getsaccessibilitieswrong), #187 (downloadRepositoryignores whether the copy succeeded).Closes #84.
🤖 Generated with Claude Code