-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
upgrade to MSL4 #45
Comments
just commit results of conversion script this is for #45
just commit results of conversion script this is for #45
What aboout #33 then? |
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 |
@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 |
Sounds good. For the old release (which I recommend to make) you should add
to the top package, so that Modelica package managers are aware of the version definition. Models using it will then have
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 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
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
|
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:
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. When I see above the Modelica dependency specified down to the patch level ( 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. |
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
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 A3: hence, the "least common denominator" here will be 4.0.2, and that will be used.
See above, |
Sure, I agree of course.
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 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. |
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 |
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. |
Thanks for the clarification, all clear! |
@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. |
@casella We have #33 for this conversion issue - for more than 22 months already. |
Back from vacation today (ten nice days in Sicily). |
it has been released long time ago
The text was updated successfully, but these errors were encountered: