pipeline: Generate the controlled term abstract class per model version, and build the layout the toolbox expects - #151
Open
ehennestad wants to merge 14 commits into
Open
Conversation
Follow the base class rename in openMINDS_MATLAB#124 so generated code matches the runtime classes it extends: openminds.abstract.Schema -> openminds.Node openminds.abstract.ControlledTerm -> openminds.base.ControlledTerm openminds.internal.abstract.MixedTypeSet -> openminds.base.MixedTypeSet openminds.base.TypesEnumerationBase -> openminds.base.TypesEnumeration openminds.base.ModulesEnumerationBase -> openminds.base.ModulesEnumeration Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two defects in _get_display_label_method_expression: Property references were qualified with a plain substring replace applied in list order, so a short property name was also rewritten inside a longer one that contains it. With ["minValue", ..., "minValueUnit"], the first pass turned "minValueUnit" into "obj.minValueUnit" and the later pass for "minValueUnit" prefixed it again, emitting "obj.obj.minValueUnit". Matching is now anchored on identifier boundaries and skips names preceded by a dot, which also protects the namespace segments of a qualified function call. The assignment to the output variable was injected only into lines containing "sprintf", a proxy that held only because every configured expression happened to use it. A stringFormat that is a plain function call produced a getDisplayLabel that never assigned str. The two JSON shapes now carry the distinction: a scalar stringFormat is an expression and is assigned here, a list is a block of statements and assigns the output variable itself, since control flow lines cannot be assigned from. The one list entry, quantitativeValue, is updated accordingly and its generated code is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The translator turns instanceDisplayConfig.json entries into the body of each generated getDisplayLabel method, and nothing exercised it. The regressions it is prone to are silent: the generated code still looks plausible and only fails once MATLAB runs it. The tests drive each stringFormat shape through an injected config rather than the checked-in one, so they cover shapes no entry currently uses and do not break when an entry is retuned. Two invariants are asserted against the checked-in config itself: every entry generates code that assigns the output variable, and none double-qualifies a property reference. Run in CI ahead of the build, so a broken generator fails before it can produce classes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The properties shared by every controlled term differ between model versions: v1.0 and v2.0 carry ontologyIdentifier, v3.0 through latest replace it with interlexIdentifier, knowledgeSpaceLink, preferredOntologyIdentifier and synonym, and v5.0 replaces those in turn with otherCrossReference, otherOntologyIdentifier and preferredCrossReference. The base class holding them was maintained by hand as two variants selected at runtime by a version threshold, so v1.0 and v2.0 resolved to a base declaring properties their model does not have while omitting ontologyIdentifier, which their generated classes document but never declare. The property set is already derived from the schemas to decide which properties a controlled term class declares itself. It now also renders the base class, emitted per version to target/base/<version> next to the types, so the base and its subclasses come from one source and stay consistent by construction. Generated output matches the hand-written variants property for property across every version, adding the mustBeMinLength validators the hand-written files omitted and the full descriptions from the schemas. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A model version can rename the properties a display label is built from,
and a schema has one entry in instanceDisplayConfig.json for every
version. The affiliation entry named memberOf, which only v3.0 and v4.0
declare, so v1.0, v2.0 and v5.0 silently fell back to a label the schema
cannot produce. contribution had the same problem for v1.0 and v2.0.
An entry may now be a list of alternatives, tried in order, the first one
whose properties the schema declares winning. Selection is by property
rather than by version number, so a new model version resolves on its own.
Character literals are also left alone when qualifying property names, so
a format string may use a property name as text. Without this,
sprintf('%d values (%s)', numel(values), unit) rewrote the word in the
format string and emitted '%d obj.values (%s)'.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fifty-eight metadata types fell through to createLabelForMissingLabelDefinition, because they carry none of lookupLabel, fullName, identifier or name and had no configured label. Most are the mathematical shapes, the annotation and relation types, and the small embedded types such as Membership, SpecimenAge and Measurement. Each is labelled from the properties that identify it, preferring ones the schema marks required and that every model version declares. Six types renamed properties between versions and use alternatives: affiliation, contribution, coordinatePoint, ellipse, rectangle and quantitativeValueRange. quantitativeValueRange also gains the range formatting it was configured for, including a fallback for v1.0 and v2.0 where both bounds share a single unit property. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…erty maps The generated ControlledTerm base constructor restricted name-value arguments to the properties it declares, so a subclass with schema-specific properties (TermSuggestion in every model version) could not be constructed with them. The constructor now mirrors openminds.Node: an unvalidated instanceSpec plus repeating name-value pairs, folded into a struct for the existing initializer. Every generated controlled term class now emits its own LINKED_PROPERTIES and EMBEDDED_PROPERTIES, which openminds.Node declares abstract, so a term with linked properties can map them. The base class declares neither. Requires the hand-written openminds.base.ControlledTermBase to drop its concrete property maps. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The property sort key meant to lead with the naming properties was looked up on bare property names while receiving full openMINDS property IRIs, so it never matched and ordering was alphabetical throughout. Fix the lookup and make the priority explicit, but leave the key unapplied: adopting it would reorder the property block of every generated class, which is a separate decision. The sort in _extract_template_variables now states the alphabetical order it relies on. A type name declared by two schemas was resolved to whichever schema sorted first and left out of the Types enumeration without a word. openMINDS type names are expected to be unique across modules, so both sites now warn and name the schema that lost, which surfaces BehavioralTask in v1.0. Verified byte-identical generated output across all six schema versions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…scape option A number constrained to a range was documented as (1,:) while every other scalar that maps a null was documented as (1,1). The list declaration is only there because a MATLAB (1,1) property requires a value; a validator constrains it back to a scalar, so the docstring should state the real cardinality. Only Frustum.minorBaseScale is affected, in latest and v5.0. The autoescape parameter never worked, because select_autoescape takes a collection of file extensions rather than a flag, so both True and False raised TypeError. Autoescaping is HTML escaping and would corrupt the generated MATLAB, so it is now off explicitly rather than by way of the template files not being named .html. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Schema source paths were parsed twice, once in the translator and once in utils, with different sentinels for a missing group and both splitting on a literal forward slash. save_resource_files re-derived the same parts a third way, by string-replacing a hard-coded "/_sources/openMINDS/schemas/<version>/" prefix out of an absolute path. All three now go through parse_schema_file_path, which uses os.path.relpath and the platform separator, and returns the module, group and file name as the path spells them. namespace_name turns a module or group into the MATLAB namespace segment, which is what keeps the "non-atlas" group reaching MATLAB as "nonatlas" while the manifest still reports it as the model spells it. save_resource_files takes the schemas root as an argument instead of assuming where the caller cloned the sources, and no longer sorts the caller's list in place. The schema payload is read as UTF-8 rather than the platform default, and the resource files are written the same way. Removed: the unused imports warnings, defaultdict, GitCommandError and OPENMINDS_VOCAB_URI, the unused TEMPLATE_FILE_NAME constants, the unused _to_label, and four blocks of commented-out code. Verified byte-identical generated output across all six schema versions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three reads were repeated for every schema the build translated: the display
label configuration file, the property set of every controlled term schema,
and the recursive walk of a version's instance folders. The walk was the
expensive one, because it was repeated for each of the hundred-odd controlled
terms in a version.
Each is now done once per version and cached. Instances are matched by their
parent folder name in memory rather than by a glob pattern per schema, which
gives the same result because an instance belongs to the schema whose folder
holds it.
For a full build of all six versions:
wall time 9.04 s -> 4.56 s
json.load calls 45190 -> 2899
glob calls 968 -> 36
seconds in glob 6.43 -> 0.20
The controlled term schemas are now read in sorted order, so the base
property set no longer depends on the order the file system lists them in.
The display label tests replace the loader rather than json.load, because the
configuration is now read once, and the controlled term tests clear the cache
between temporary schema trees.
Verified byte-identical generated output and console output across all six
schema versions.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The build wrote one folder per artifact type, each holding a subfolder per model version, and applying it took a copy per type plus a rule that mapped the controlled term base onto the two variants the toolbox kept by hand. The toolbox now keeps its generated classes at code/generated/resources, model version first, with the controlled term base generated alongside the types, mixed types and enumerations of that version. target/ takes the same shape, so applying a build is one copy of the whole tree, and the base class needs no special handling at all. target_folder is the one place that knows how the tree is laid out. The apply script replaces every model version rather than the contents of three folders, so a version the model has dropped does not linger in the toolbox, and it restores both overlays with one copy each now that they are laid out like their destination. Verified that the reorganized build has the same contents as before, folder for folder, and that applying it onto the toolbox branch reproduces the generated classes committed there exactly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…dule The abstract class holding the properties a version's controlled terms share was emitted as openminds.base.ControlledTerm, into a "base" folder of its own. It describes the controlled terms module, not the toolbox machinery, so it is now openminds.controlledterms.ControlledTerm and is written with the other classes of that module. The concrete terms extend it, and it extends openminds.base.ControlledTerm, which is what the hand-written class in the toolbox is renamed to. That removes the fourth artifact type: a version folder holds types, mixedtypes and enumerations, and the abstract class is just another generated class in the types tree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The shape of target/ is a contract with the toolbox now that applying a build is a single copy, so the readme says what the build writes, why the model version comes first, and where the abstract controlled term class goes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ehennestad
changed the title
ehennestad
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.
Pipeline side of #150. The toolbox keeps its generated classes at
code/generated/resources/<model version>/<kind>and expects the class every controlled term extends to be generated per version; this builds that. The two need to land together, because neither works against the other side's old state.The controlled term classes
The abstract class holding the properties a version's controlled terms share is generated per model version, because that property set differs between versions. It is emitted as
openminds.controlledterms.ControlledTerm, with the other classes of its module, and extendsopenminds.base.ControlledTerm, the hand-written class #150 renames the machinery to. The concrete terms extend the generated one.TermSuggestionexposed two problems with how the generated classes used it. Its constructor forwarded every name-value pair to the base constructor, whose arguments block only accepted the base class's own properties, soTermSuggestion(addExistingTerminology=...)failed withInvalid argument name. The generated class now takes repeating name-value pairs the wayopenminds.Nodedoes.Every generated controlled term also emits its own
LINKED_PROPERTIESandEMBEDDED_PROPERTIES.openminds.Nodedeclares both abstract, and a term with a linked property has to be able to declare them. Neither of the classes above it defines them, which is the matching change in #150.Build layout
target/takes the shape of its destination, model version first, so applying a build is one copy of the whole tree:target_folderis the one place that knows the layout. The abstract controlled term class needs no folder of its own: it is written into the types tree as another class of its module. The apply script replaces every model version rather than the contents of three folders, so a version the model has dropped does not linger in the toolbox.The readme now describes what the build writes, why the model version comes first, and how a build is applied.
Correctness fixes found along the way
A property sort key meant to lead with the naming properties was looked up on bare property names while receiving full property IRIs, so it never matched. It is fixed but deliberately not applied: adopting it would reorder the property block of every generated class, which is a separate decision.
A type name declared by two schemas was resolved to whichever sorted first and left out of the
Typesenumeration without a word. Both sites now warn and name the class that lost, which surfacesBehavioralTaskin v1.0.A number constrained to a range was documented as
(1,:)while every other scalar that maps a null was documented as(1,1). The list declaration exists only because a MATLAB(1,1)property requires a value.The
autoescapeoption never worked, becauseselect_autoescapetakes a collection of file extensions rather than a flag. Autoescaping is HTML escaping and would corrupt the generated MATLAB, so it is off explicitly now.Housekeeping
Schema source paths were parsed three different ways, one of them by string-replacing a hard-coded
_sourcesprefix out of an absolute path, and all of them splitting on a literal forward slash. They now go through one parser that uses the platform separator, which also keeps thenon-atlasgroup reaching MATLAB asnonatlaswhile the manifest still spells it as the model does.Three reads were repeated for every schema: the display label config, the property set of every controlled term schema, and the recursive walk of a version's instance folders. Each is now done once per version. A full build of all six versions drops from about 9 seconds to about 4.5, with 45,190 JSON reads becoming 2,899.
Dead code removed: four unused imports, two unused constants, one unused function and four blocks of commented-out code.
Tests
The suite grows from 27 to 70 cases, covering the controlled term classes and the abstract class they extend, the schema path parser, the duplicate type name reports, the property size contract and the instance lookup.
🤖 Generated with Claude Code