EDF-Lab / BuildSysPro

BuildSysPro open source: EDF's Modelica library for buildings, districts and energy systems modelling
Other
42 stars 22 forks source link

Issues with MSL 4.0.0 and BuildSysPro #11

Closed casella closed 1 year ago

casella commented 1 year ago

@Kodsgr, @bcharrier, we are testing BuildSysProp with OpenModelica, report here. Until commit 3deab2b everything was more or less fine, then we got a huge regression going from that to 9ed96c5b, when you updated the library to use MSL 4.0.0.

Unfortunately, it seems that the conversion didn't go well, as most of the models fail with errors such as

[BuildSysPro 3.5.0-master/IBPSA/BoundaryConditions/SolarGeometry/ZenithAngle.mo:4:3-4:50:writable]
Error: Class Modelica.SIunits.Angle not found in scope ZenithAngle.

Modelica.SIunits.Angle is a MSL 3.2.3 type, the MSL 4.0.0 type is Modelica.Units.SI.Angle.

It seems that the version on the master branch was actually not converted, and still uses the old MSL. Please note that changing the version number in the uses annotation is not enough to make the library work with MSL 4.0.0, you need to run the conversion scripts on it, so that all references to classes that changed names are updated. Apparently, something went wrong with that.

Can you please check?

Also, if you follow the discussion in modelica/ModelicaSpecification#3209, there seems to be a solid consensus that if a library moves from using MSL 3.2.3 to using MSL 4.0.0 it is not backwards compatible, so if you want to use Semantic Versioning, you should probably make a major release (4.0.0), not a minor release (3.5.0). Anyway, this is a matter of taste, while the issue with old MSL 3.2.3 types is real and should be fixed ASAP.

bcharrier commented 1 year ago

Hi @casella Thank you for your feedback ! It seems that the majority of the errors concern the sub-package "IBPSA". This sub-package is developed by a working group from the IBPSA association. It was decided with other partners (Buildings, AixLib, IDEAS, BuildingSystems) that each building energy library would distribute it to end-users as part of their respective library. So we are not the maintainers of the IBPSA sub-package. When releasing version 3.5.0 of BuildSysPro whe integrated the last version of the IBPSA sub-package available at this date, and as you see there are some conversion errors regarding MSL 4.0.0. We will update the version of this IBPSA sub-package when releasing the next version of BuildSysPro. Cc @Kodsgr

casella commented 1 year ago

@bcharrier thanks for the explanation, but I'm afraid this doesn't really work well.

According to the Modelica Language Specification, Section 18.8.2, the uses annotation only applies to top-level classes. Hence, you can't have package X using MSL 4.0.0, with sub-package X.Y using another version of MSL. The sub-library is unusable (most models use SI units, so they are broken), hence there's no point shipping it. Unless you want to send out to the users of your library a very loud message that the IBPSA group is lazy and hasn't updated it yet, by making them angry when they see nothing works, but I guess that's not your intention 😄. Furthermore, most users will have no idea about this deal, so they would probably blame EDF, who distributes the library, instead of the IBPSA working group.

I believe the only sensible decision in this context is to delay the release of libraries that contain IBPSA until it's been updated to MSL 4.0.0, including BuildSysPro 3.5.0. That shouldn't be such a big deal, the conversion can be carried out automatically, using the conversion scripts provided with MSL 4.0.0.

@mwetter, what do you think?

casella commented 1 year ago

@dietmarw, @beutlich, this is one of the reasons why non-backwards-compatible MSL versions should only be issued once in a decade (at most). Even if the conversion is fully automatic. The problem is, inter-dependent libraries developed across different organizations can create unwanted deadlock situations.

dietmarw commented 1 year ago

@casella As you pointed out, laziness is a bad excuse for stopping progress.

mwetter commented 1 year ago

The situation seems to be caused by BuildSysPro having copied the IBPSA Library into their package at https://github.com/EDF-Lab/BuildSysPro/blob/master/BuildSysPro/IBPSA/package.mo The copied version is from 2018 (a long time ago during which many bugs were fixed): https://github.com/EDF-Lab/BuildSysPro/blob/9ed96c5b1d3cdc2bdfbe2c3fb711c2bec3b403f4/BuildSysPro/IBPSA/package.mo#L6 This is well before the master of the IBPSA Library was updated to MSL 4.0.0.

BuildSysPro could be fixed by updating BuildSysPro/IBPSA with any recent version of the master.

The derivative libraries (AixLib, Buildings, BuildingSystems, IDEAS) don't experience this problem because they regularly update their libraries with the version of the master, using the script https://simulationresearch.lbl.gov/modelica/buildingspy/development.html#module-buildingspy.development.merger This is the process that we developed within the development group of the IBPSA Library.

Making an new release of IBPSA, while generally a good thing, won't resolve this issue as BuildSysPro has a static copy of the library. We haven't done official releases since a while as the above derivative libraries frequently copy latest versions from the IBPSA master as part of their development process, which often involves updating their library or first contributing to the IBPSA library if changes are needed upstream. If EDF actively develops their library, they would be more than welcome to join again the development of the IBPSA library.

bcharrier commented 1 year ago

Ok thank you @mwetter, we didn't dare to integrate a non stable version of IBPSA. It is corrected I have just updated the package with the latest version from IBPSA master. It would of course be a pleasure to contribute more actively to the developments of IBPSA, but we have run out of time in recent years... This could come back, work on humidity and air quality will be published during the year and will, I hope, offer opportunities for collaboration.

casella commented 1 year ago

All's well that ends well, see report.

Thanks!