HelmholtzMedia
HelmholtzMedia copied to clipboard
thorade
upgrade to MSL4
Did this ticket and commit for a git demo today. Thanks for the reminder, I will take a look at #33 also!
This broke some of the models since they use models from Modelica.Media.Examples which are not covered by the conversion script (I guess they're not really meant to be used by users). See this report.
@thorade, I would suggest that you make a proper release with the previous version using MSL 3.2.3, so people can still use it if needed. Then, I'd also make one using MSL 4.0.0.
If you want to use semantic versioning, which is recommended for Modelica Libraries, they could be 1.0.0 and 2.0.0 (since they are not compatible, due to the different MSL dependencies). You should also add version annotations to the top package for completeness.
I can help with that if you need.
For now, I have just reverted the commit to master, so that master is back at MSL 3.2.3 The MSL4 version is in the branch https://github.com/thorade/HelmholtzMedia/tree/msl4update I will fix the tests in that branch before merging it into master
Sounds good. For the old release (which I recommend to make) you should add
annotation(version = "1.0.0");
to the top package, so that Modelica package managers are aware of the version definition. Models using it will then have
annotation(uses(HelmholtzMedia(version = "1.0.0")));
and will know how to get the library automatically. This annotation is added automatically by OMC and by Dymola if you use stuff in a model that comes from a library that has the version annotation.
For the new release, the one using MSL 4.0.0, the question is whether user models that used the old MSL 3.2.3 version of HelmholtzMedia need any adaptations to run with the the new version, except of course switching to MSL 4.0.0. As far as I understand, this is not the case. Of course HelmholtzMedia will be different, because it uses a new MSL, which impacts, e.g., where SI units are stored. But the way you use it shouldn't have changed at all.
What I mean is that if you want to upgrade your models to MSL 4.0.0 and to the new HelmholtzMedia using it, you need to upgrade your models to MSL 4.0.0 with the MSL conversion script, but then there is no further need to touch anything regarding HelmholtzMedia library references in them.
This means that the new HelmholtzMedia is in some sense backwards-compatible with the old one, in the sense that you don't need any conversion script for it specifically, so you could use the noneFromVersion annotation to declare it explicitly.
However, I'm not sure we can say that the new HelmholtzMedia is truly backwards-compatible, because it needs the models using it to upgrade to MSL 4.0.0, which is non-backwards-compatible with MSL 3.2.3 and requires irreversible changes. So, I'm not sure whether the new version should be 1.1.0 or 2.0.0 according to semver.
@sjoelund is working on the OMC package manager, that handles these issues, he may want to comment on that. We are currently designing the conversion manager for OMC, this is a nice use case to make sure we do everything right. Of course to test is we'll need the two releases 😄
@dietmarw is also an expert on semver, what does he think?
The underlying issue is multiple dependencies in Modelica models, which hasn't really been sorted out completely. If your library A uses libraries B and C, and both use library D, there are three possibilities, in case B and C use different versions of D
- load two different versions of D and have B and C each use its own
- give an error and quit
- try to find some least-common-denominator versions of B and C that use the same D, and then load them
I'm not sure that technically speaking the Modelica Language Specification prevents the first option explicitly, but for sure the GUI's I am aware of assume that only one version of each library is loaded at the same time, and I guess this is sensible to avoid headaches, even though it induces serious constraints. The second option may be a bit too restrictive. The question is if anyone has ever considered the third option from a theoretical point of view. This may also be an interesting question for @mtiller
There should be a new major version even if there were no changes applied by the conversion script. Simply because Modelica does not explicitly allow two different MSL versions to be loaded and you don't know if the user's model uses anything from MSL that is not backwards compatible. It could be that some Modelica tool would handle multiple MSL versions loaded, but it's safer to assume that they don't.
Good argument. I guess we should sort this out explicitly in the spec, but that is a separate issue.
So the MSL 4 version should have
annotation(uses(Modelica(version="4.0.0")),
version="2.0.0",
conversion(noneFromVersion="1.0.0"));`
I'm not sure it's supposed to be noneFromVersion. OpenModelica doesn't check for what's needed in a local conversion script. If you inherit from a class that uses a component that is renamed, you need conversion rules for this as far as I know.
I'm not 100% sure either, but I believe the interfaces of Modelica.Media (which is all HelmholtzMedia needs, besides SI units) haven't really changed at all, so I think this should be the case here.
We need to check, maybe @thorade could have a look.
Just a comment on this remark:
The underlying issue is multiple dependencies in Modelica models, which hasn't really been sorted out completely. If your library A uses libraries B and C, and both use library D, there are three possibilities, in case B and C use different versions of D
- load two different versions of D and have B and C each use its own
- give an error and quit
- try to find some least-common-denominator versions of B and C that use the same D, and then load them
The question is if anyone has ever considered the third option from a theoretical point of view.
Typically, this consideration is the domain of package managers (like pip, conda, apt, etc), as you are probably aware, and one tool that is used is SAT solvers.
In general, it's a complex problem to solve efficiently. From what I have seen, it remains tractable when the dependency specifications used in libraries are as loose as possible and as strict as needed (package A: numpy 1.*, package B: numpy>=1.4.2, e.g. because some bug was fixed in 1.4.2).
When I see above the Modelica dependency specified down to the patch level (4.0.0), it does not look like this will be generally solvable - what if one dependency in your project demands Modelica 4.0.0 and another Modelica 4.0.2?
Although only a patch-level difference (so intuitively compatible), the specificity of the first version string does not allow another version than that precise one. I'm wondering: should that rather be uses(Modelica(version="4")), since you only care about major (i.e. compatibility-breaking) version changes?
A more generic way to solve this would be to admit version strings with boolean (and other) specifiers, like package managers typically do, but that is probably more a thing for the Modelica Specification.
Typically, this consideration is the domain of package managers (like pip, conda, apt, etc), as you are probably aware, and one tool that is used is SAT solvers.
Sure. The actual question was if anyone has considered it within the MAP_LANG group, which is responsible for the development of the Modelica Specification.
@dietmarw and @mtiller wrote a tool, named impact, and wrote extensively about it. Unfortunately, it was not embraced by the community. It didn't handle external libraries, which is a critical limitation as far as I am concerned.
In general, it's a complex problem to solve efficiently. From what I have seen, it remains tractable when the dependency specifications used in libraries are as loose as possible and as strict as needed (package A:
numpy 1.*, package B:numpy>=1.4.2, e.g. because some bug was fixed in 1.4.2).When I see above the Modelica dependency specified down to the patch level (
4.0.0), it does not look like this will be generally solvable - what if one dependency in your project demands Modelica 4.0.0 and another Modelica 4.0.2?
A1: One should always use the latest patch version. Even if it changes a regression, because it's likely that the old result was wrong and the new is better
A2: we do have one annotation for libraries conversion(noneFromVersion="x.y.z") that explicitly states there is no need to convert your models to use the newer patch library. We already do that in OMC, if you open a model that uses MSL 3.2.2, it will load MSL 3.2.3 instead, which has conversion(noneFromVersion="3.2.2"), and just issue a notification.
A3: hence, the "least common denominator" here will be 4.0.2, and that will be used.
Although only a patch-level difference (so intuitively compatible), the specificity of the first version string does not allow another version than that precise one. I'm wondering: should that rather be
uses(Modelica(version="4")), since you only care about major (i.e. compatibility-breaking) version changes?
See above, noneFromVersion does the trick.
A1: One should always use the latest patch version.
Sure, I agree of course.
A2: we do have one annotation for libraries conversion(noneFromVersion="x.y.z")
Ah, I get it now, this is an interesting mechanism!
@thorade, we're now testing the conversion of HelmholtzMedia to MSL 4.0.0, using OpenModelica. As reported here, we are getting a large number of failures because you are using Modelica.Media.Examples.Tests.Components.PartialTestModel that has no conversion rules in the MSL 3.2.3->4.0.0 conversion script.
Either you can find a way to avoid using it, or we should add a rule to the MSL scripts. What do you think?
Just a comment here. Examples in the MSL have no conversion rules on purpose and by design since people should not rely on those examples by extending from it. If this was done here, then it needs to be fixed in the library. MSL will not suddenly add conversion rules for examples since this would open a whole lot of other problems. The easy fix for the library is to simply duplicate the example they extended from before and do the modifications/adaptations on the copy. That will get converted fine (as long as the copied example class itself does not rely on other example classes).
by design since people should not rely on those examples by extending from it. If this was done here, then it needs to be fixed in the library.
Not the library author, but afaict the MSL itself is creating media test models/examples (e.g. Media.Examples.ReferenceAir.DryAir1) by extending from Modelica.Media.Examples.Utilities.PartialTestModel, so the admonition that one should not do that to create very similar media test models in a library seems strange to me on the face of it.
From a few spot checks it seems usage herein mirrors what is done in MSL(4). Is there more involved in the fix than pointing to the new home of PartialTestModel (now Modelica.Media.Examples.Utilities), or is the situation more complex, e.g. because this partial model was actually moved into ModelicaTest (I see another PartialTestModel in there)?
Copy-pasting model code around triggers my personal DRY sensor, I have to admit. 😝
The key here is that in the MSL an example is extending from another example. And the MSL devs are well aware that those models need to get updated manually. Should you find any normal component outside an example package that extends from inside an example then that would be a mistake. But I'm pretty sure there is none.
I share your feelings about copy-pasting and of course other libraries can extend from examples from the MSL they just need to be aware that they will have to upgrade to new versions manually as there will not be a conversion script given for any of the example models.
@dietmarw this criterion (thou shalt not extend from examples) is good for me, but are we declaring it explicitly somewhere?
Looks like only in some MAP-LIB meeting minutes. So should probably be added to the User's Guide at some point.
Well. A conversation rule could be added even though it's not actually supported. It was just moved in this case, right? Fixing it proper here would be a nicer solution though.
@thorade, we're now testing the conversion of HelmholtzMedia to MSL 4.0.0, using OpenModelica. As reported here, we are getting a large number of failures because you are using
Modelica.Media.Examples.Tests.Components.PartialTestModelthat has no conversion rules in the MSL 3.2.3->4.0.0 conversion script.Either you can find a way to avoid using it, or we should add a rule to the MSL scripts. What do you think?
@casella We have #33 for this conversion issue - for more than 22 months already.
Back from vacation today (ten nice days in Sicily). The test model is quite simple, just a source, volume, pipe and sink, so probably a clean solution could be to copy it to the library or create a similar simple model. The test cases are not very clean anyway, they were mostly created for manual testing or as examples, and many of them are only run for one fluid, so the cleaner solution would be to convert all into basic tests that are then extended for each medium, but then it is a bit more work to fix it and also it would result in quite many tests.