Opened 6 years ago
Last modified 3 years ago
#5297 assigned enhancement
Support of conversion scripts
Reported by: | Francesco Casella | Owned by: | Per Östlund |
---|---|---|---|
Priority: | blocker | Milestone: | 1.19.0 |
Component: | Interactive Environment | Version: | |
Keywords: | Cc: | Niklas Worschech, Martin Sjölund |
Description
The decision of the MAP-LIB group for the next release of the MSL is that we make a major 4.0.0 release with a thorough clean-up, that will require the use of conversion scripts to update other libraries to this new version. The requirement is that the conversion can be made 100% automatic with the available conversions in the Modelica specification. This will probably take about two years before the release.
This means OpenModelica will need to support conversion scripts to work with the next MSL version. I think we should have this feature ready for omc 2.0.0, though we could postpone it to 2.1.0 based on the status of MSL 4.0.0 at the time of the omc 2.0.0 release.
In fact, I already proposed to the MAP-LIB group that we use the OpenModelica CI infrastructure to test that the provided conversion scripts are fine, by running them on all the open-source libraries that we have in the testsuite and then testing them with MSL 4.0.0-development. Once we have the scripts implemented, I understand this feature requires very little setup work of Hudson/Jenkins, although it may require some additional hardware resources - we could ask the MA to pay for that, and offer this service to the MAP-LIB developers.
Change History (22)
comment:1 by , 6 years ago
Component: | New Instantiation → Interactive Environment |
---|---|
Owner: | changed from | to
Type: | defect → enhancement |
comment:2 by , 5 years ago
Cc: | added |
---|---|
Owner: | changed from | to
Status: | new → assigned |
comment:3 by , 5 years ago
You miss the fact that in your fancy library:
package P import MB = Modelica.Mechanics.MultiBody; model M MB mb; // This needs to be recognized as changing... end M; annotation (uses(Modelica(version="3.2.1"))); end P;
So it's not a trivial thing; it requires some lookup rules to figure out which identifiers referred to the now not loaded Modelica 3.2.1.
comment:4 by , 5 years ago
Yeah. Basically you need to expand imported packages, run the scripts on the expanded names, and then shrink them back to use the imported symbols, or something like that.
In fact, this would be a sound, bullet-proof implementation. However, I looked up all instances of import
in the MSL (there are 1070), and as far as I see the likelyhood that we have changed names in imported classes is very, very low.
So I think a first crude version of this functionality that simply ignores imported symbols will work 99% (or 99.9%) of the times, which would be much better than not having anything at all. In case of failure, the model will fail the compilation, so again no bigger harm than not having this functionality at all.
My suggestion:
- start by implementing a basic version not supporting import
- later refine it by adding proper handling of class references using imported symbols
follow-up: 7 comment:5 by , 5 years ago
Owner: | changed from | to
---|
@perost, you may want to have a look at this. According to the current schedule MSL 4.0.0 should be released by April (possibly a bit later), so we could already include it in 1.16.0, as long as we can provide automatic conversion of 3rd-party libraries to the new MSL by conversion scripts.
How long do you think developing this feature could take?
comment:6 by , 5 years ago
It was "ready next week" about 11 years ago, so I guess just a little longer.
follow-up: 8 comment:7 by , 5 years ago
Replying to casella:
How long do you think developing this feature could take?
I'm not sure. A naive implementation that operates on SCode without doing any lookup might be relatively simple to implement, maybe a couple of days or so.
follow-up: 9 comment:8 by , 5 years ago
Replying to perost:
I'm not sure. A naive implementation that operates on SCode without doing any lookup might be relatively simple to implement, maybe a couple of days or so.
The plan for MSL 4.0.0 is to make sure that one can update his/her existing models using MSL 3.2.3 in a 100% automatic way, no questions asked.
The question is, is such a naive implementation going to work in most cases, and produce clear and meaningful errors when it doesn't, or is it going to mess up with end users' libraries in a possibly irreversible way and possibly with the users not knowing until they try the model after one year and find it broken?
The former outcome is ok for me: if it works, fine, otherwise use another tool. No harm done. The latter outcome must be obviously be avoided at all costs.
@perost, what do you think?
follow-up: 10 comment:9 by , 5 years ago
Replying to casella:
The question is, is such a naive implementation going to work in most cases, and produce clear and meaningful errors when it doesn't, or is it going to mess up with end users' libraries in a possibly irreversible way and possibly with the users not knowing until they try the model after one year and find it broken?
I'm really not sure. Applying the conversion rules naively is mostly just a matter of search/replace, with the added complication that classes are sometimes moved to other packages (which might need to be created).
But this only works for fully qualified names and not imported names as pointed out, although I guess applying the conversion rules to the imports might be enough in many cases (like import SI = Modelica.SIunits
=> import SI = Modelica.Units.SI
). But I'm not familiar enough with how different libraries use imports to say whether this will work for most of them.
If we do implement this we will need to carefully consider how to use it though. We should probably let the user decide when they want to convert their libraries/models rather than doing it automatically for them.
comment:10 by , 5 years ago
Replying to perost:
I'm really not sure.
Then we should probably try and see what happens. We can run the scripts through the entire larger testsuite (which has many thousand models) and look for regressions.
Applying the conversion rules naively is mostly just a matter of search/replace, with the added complication that classes are sometimes moved to other packages (which might need to be created).
But this only works for fully qualified names and not imported names as pointed out, although I guess applying the conversion rules to the imports might be enough in many cases (like
import SI = Modelica.SIunits
=>import SI = Modelica.Units.SI
).
I also guess so. We haven't deliberately tried to come up with complicated name changes, just straightforward ones. Imports are definitely needed, since we changed the name for the units package, which is imported everywhere. Other imports are typically used for enumeration type in choice menus, what @perost suggests should be enough.
If we do implement this we will need to carefully consider how to use it though. We should probably let the user decide when they want to convert their libraries/models rather than doing it automatically for them.
Absolutely! The decision whether or not to convert from MSL 3.2.3 to MSL 4.0.0 is definitely up to the user, but then the implementation of the decision needs to be carried out automatically.
@perost, pls. let me know when you can schedule those few days.
follow-up: 15 comment:13 by , 4 years ago
Initial conversion script support is now implemented in PR 7273 (might take a while to merge though, we seem to have some server issues at the moment).
For the initial support there's currently a convertPackage
script function which takes a package and the path to a conversion script file (e.g. convertPackage(Buildings, "/path/to/MSL/ConvertModelica_from_3.2.3_to_4.0.0.mos")
). We obviously need to implement something more user friendly, but what exactly that will be is something I guess we'll need to discuss.
There's also a flag -d=dumpConversionRules
that prints out the conversion rules in a needlessly elaborate manner when calling convertPackage
that can be used for debugging.
I've used the Buildings library for testing, and at the moment I only know of one issue. That issue is that Buildings.Media.Specalized.Air.PerfectGas
inherits from Modelica.Media.Interfaces.PartialCondensingGases
and extends the gasConstant
function. The conversion script for MSL 4.0.0 has several rules for converting R
to R_s
in the various Medium packages but not for PartialCondensingGases
, so converted models using that function become incorrect (3 models failing because of this I think). I assume this is an oversight in the conversion script for MSL 4.0.0 since conversion should be possible without access to the old library.
comment:15 by , 4 years ago
Replying to perost:
I've used the Buildings library for testing, and at the moment I only know of one issue. That issue is that
Buildings.Media.Specalized.Air.PerfectGas
inherits fromModelica.Media.Interfaces.PartialCondensingGases
and extends thegasConstant
function. The conversion script for MSL 4.0.0 has several rules for convertingR
toR_s
in the various Medium packages but not forPartialCondensingGases
, so converted models using that function become incorrect (3 models failing because of this I think). I assume this is an oversight in the conversion script for MSL 4.0.0 since conversion should be possible without access to the old library.
Would you mind opening an issu on the MSL github tracker?
comment:16 by , 4 years ago
I would suggest that we add a task to Jenkins that runs the conversion on all the OS libraries using MSL 3.2.3 that we have in the testsuite, runs all the models and then issues a regression report, comparing the results with MSL 3.2.3.
This should be run every once in a while, to make sure that conversion scripts are not broken, and also to possibly test for new corner cases coming with new libraries added to the testsuite.
follow-ups: 18 19 comment:17 by , 4 years ago
So the following libraries failed without error-message:
"AixLib", "FCSys", "SiemensPower", "ScalableTestSuite", "ThermoSysPro", "ThermoPower"
Then there is a problem with UTF-8 and trying to save/diff the converted files to disk. That can be dealt with by preprocessing the files (converting them outside of OpenModelica). However, it would need some extra work to automatically handle this.
The next problem is non-standard files, that is supported without warnings or errors. Such as classes with multiple annotations or annotations not at the end of a class. These fail miserably, and when trying to add support to them in the diff algorithm the diffs blow up and use 100% of RAM. So libraries like this also need to be converted in multiple steps, or the diff algorithm skipped.
This is also a problem for OMEdit since it will mess up files with annotations in the wrong place.
comment:18 by , 4 years ago
Replying to sjoelund.se:
Then there is a problem with UTF-8
Which problem, exactly? UTF-8 being used, or not being used?
The next problem is non-standard files, that is supported without warnings or errors. Such as classes with multiple annotations or annotations not at the end of a class. These fail miserably, and when trying to add support to them in the diff algorithm the diffs blow up and use 100% of RAM. So libraries like this also need to be converted in multiple steps, or the diff algorithm skipped.
This is also a problem for OMEdit since it will mess up files with annotations in the wrong place.
Q1: Are annotations in the "wrong" place explicitly not allowed in the specification
Q2: Should we try to automatically fix them?
comment:19 by , 4 years ago
Replying to sjoelund.se:
So the following libraries failed without error-message:
"AixLib", "FCSys", "SiemensPower", "ScalableTestSuite", "ThermoSysPro", "ThermoPower"
Fixed in d093f807.
See Section 18.8.2.1 of the specification.
As I understand you need to run a script when loading a library that has a
uses
annotation referring to an older version of already loaded dependencies. This script calls a bunch of simple functions to change old class names into new class names and old modifiers into new modifiers in the models of the library being loaded. This seems to me really not a big deal. @adrpo, @sjoelund.se, do I miss something?Of course it would be nice to have another feature, i.e. automatic refactoring. If I load library
A
and then librariesB
andC
that use components fromA
, and then I change some names inA
, I'd like to have all the instances inB
andC
to be automatically updated. But that's not the topic of this ticket. Maybe this was what you were referring to during today's devmeeting?