Mint every SED-ML id from one document-wide allocator - #1846
Merged
Conversation
SED-ML SIds share a single namespace across every kind of object, but the exporter built them from several independent recipes - application names, simulation names, mangled variable names, per-kind counters - with no check that two of them hadn't landed on the same string. biomodel_28625786 has an application AND a simulation both named "compartmental", so the archive carried <model id="compartmental"> and <uniformTimeCourse id="compartmental">, which the OMEX validator rejects: "Each identified SED object must have a unique id". #1840 fixed this within the application namespace only. Rather than repeat that guard per kind, route every mint through uniqueId(): a free base is returned unchanged, so ids move only where they previously collided. For biomodel_28625786 the sole difference is the simulation becoming "compartmental_sim0" - applications are reserved up front so they keep first refusal on their name, which also keeps the SBML file names they generate. This closes two latent collisions of the same family that no test had hit yet: override-derived models are <simContextId>_<overrideCount>, which can land on another application's mangled id (this very model has applications "simple_1" and "simple_1.5" -> "simple_1_5"), and data generator ids are "dataGen_<task>_<mangled var>", where the mangling is many-to-one. The 'time' data generator is now looked up by the id it was given rather than by rebuilding that id at the use site, since the allocator is free to move it.
biomodel_82065439 was recorded as OMEX_PARSER_ERRORS for the same duplicate "compartmental" id; it now round-trips cleanly, so the entry has to go or the test fails with "passed SEDML Round trip, but knownSEDMLFault was set". biomodel_28625786 still fails, but on a bug in the validator rather than in the archive. biosimulators-utils 0.2.3 checks every repeatedTask setValue target against the LAST model in the document instead of the one the change references - sedml/validation.py:509 passes a leaked `model` loop variable where it means `change.model`. Here that lands on HPC_070907; species targets happen to exist there so they pass, the kinetic parameters do not, and the errors are then reported against models (simple_1, simple_1_5) that were never searched. Those parameters do resolve against their own model sources. Recorded as a known fault until the upstream fix lands.
Member
Author
|
Full-group verification complete — the run flagged as pending in the description has finished: All 264 models including the slow set, i.e. the full suite the nightly runs. No additional models had a With this, the nightly |
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.
Last of the nightly
SEDML_SBML_ITfailures.biomodel_28625786has an application and a simulation both namedcompartmental, so the exported archive carriedwhich the OMEX validator rejects: "Each identified SED object must have a unique id. Multiple objects have the following ids: [compartmental]".
Why the previous fix didn't cover it
SED-ML SIds share one namespace across every kind of object, but
SEDMLExporterbuilt them from several independent recipes — application names, simulation names, mangled variable names, per-kind counters — with no check that two hadn't landed on the same string. #1840 added a uniqueness guard within the application namespace only; simulation ids were still minted unguarded increateSedMLSim.Rather than repeat that guard per kind, every mint now goes through one allocator:
A free
baseis returned unchanged, so ids move only where they previously collided — a correct archive is unaffected. Applications are reserved up front (reserveSimContextIds) so they keep first refusal on their name, which matters because an application's id also names the SBML file written for it.#1840's_app<n>is now one case of the general_<kind><n>.For
biomodel_28625786the entire id diff against master is one line:Latent collisions this also closes
Neither had been hit by a test yet:
<simContextId>_<overrideCount>, which can land on another application's mangled id. This very model has applicationssimple_1andsimple_1.5→simple_1_5; a sixth override variant ofsimple_1would have collided.dataGen_<task>_<mangled var>, andTokenMangler.mangleToSNameis many-to-one — speciesa.banda-bboth mangle toa_b.The 'time' data generator is now looked up by the id it was actually given rather than by rebuilding that id at the use site, since the allocator is free to move it.
Test expectations
biomodel_82065439had the same duplicate-compartmentalfault recorded and now round-trips cleanly, so it failed with "passed SEDML Round trip, but knownSEDMLFault was set" — entry removed. (This is the usual signal on this suite: a fix surfaces as a failure.)biomodel_28625786still fails, on a validator bug rather than on the archive, and is recorded as a known fault with the diagnosis inline.biosimulators-utils0.2.3 checks everyrepeatedTasksetValuetarget against the last model in the document instead of the one the change references —sedml/validation.py:509passes a leakedmodelloop variable where it meanschange.model. Here that lands onHPC_070907; species targets happen to exist there so they pass, the kinetic parameters do not, and the errors are then reported against models (simple_1,simple_1_5) that were never searched. Verified with lxml that those parameters do resolve against their own model sources:0.2.3 is the latest release on PyPI, so there's nothing to upgrade to; the entry comes out when the upstream fix lands.
Testing
PR CI cannot exercise this —
SEDML_SBML_ITruns only inregression.yml, and these models sit behindtest.include.slow. Locally:mvn -o test -pl vcell-core -Dgroups=SEDML_SBML_IT -Dtest=SEDMLExporterSBMLTest \ -Dtest.include.slow=true -Dtest.only=biomodel_28625786,biomodel_82065439,biomodel_220138948The duplicate-id error is gone;
biomodel_82065439now passes outright. The full 264-model group is still running — any other model whose fault this resolves will announce itself the same way, and I'll push a follow-up commit if more turn up before merge.🤖 Generated with Claude Code
https://claude.ai/code/session_01SvqmME7MkRUNEYje5HpiLt