Skip to content
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

Bump used Modelica language version to 3.6 for MapleSim moparser (CI) #4235

Merged
merged 1 commit into from
Feb 5, 2024

Conversation

beutlich
Copy link
Member

According to #4175 this should not be merged right now.

@beutlich beutlich added CI Issue that addresses continuous integration requires Modelica 3.6 Issue that requires Modelica Language Specification 3.6 labels Nov 21, 2023
@beutlich beutlich changed the title Bump used Modelica language version to 3.6 Bump used Modelica language version to 3.6 for MapleSim moparser (CI) Nov 21, 2023
@beutlich beutlich marked this pull request as draft November 21, 2023 16:57
@maltelenz
Copy link
Contributor

maltelenz commented Nov 22, 2023

There was a decision in the monthly MAP-Lib meeting on the 14th of November, to switch to 3.6 fully, and consider for each PR that tries to use 3.6 features, whether we are ready to accept it for 4.1.0 or not.

Unfortunately it doesn't seem this was written down anywhere. It was discussed in the context of #4208.

So this should be possible to merge.

@tobolar tobolar requested review from maltelenz and removed request for tobolar November 22, 2023 10:56
@tobolar
Copy link
Contributor

tobolar commented Nov 22, 2023

@beutlich : I guess @maltelenz is better suited to review this PR. I was not present at the last MAP-Lib meeting.

@eshmoylova
Copy link
Member

@hubertus65 recorded the following in the chat on Teams for MAP-LIB Monthly:
image
If we say in the documentation that we support 3.6 then we need to support that claim elsewhere.

@henrikt-ma henrikt-ma marked this pull request as ready for review January 9, 2024 13:28
@henrikt-ma
Copy link
Contributor

henrikt-ma commented Jan 9, 2024

@casella, I suppose you as the project leader is the right person to push the merge button on this.

Edit: My bad, I was confused and mixed up this PR with #4208.

@arunkumar-narasimhan
Copy link
Collaborator

@casella, this PR is all ready to merge, but seems to be failing at checks. Could you please look into it?

@beutlich
Copy link
Member Author

but seems to be failing at checks.

Not any more.

Note, that merging this PR opens the door for PRs like #4231 which I'd not to have right now. Therefore I'd not like to have it merged now.

@beutlich beutlich marked this pull request as draft January 18, 2024 05:57
@henrikt-ma
Copy link
Contributor

Note, that merging this PR opens the door for PRs like #4231 which I'd not to have right now. Therefore I'd not like to have it merged now.

This sounds like a misunderstanding to me. The door for #4231 is already open version-wise as of #4208. What is needed in order to move forward with #4231 shouldn't have anything to do with the MapleSim moparser, but what we need is an agreement with tool vendors that they welcome the adoption of the specific language feature selective model extension.

Hence, I believe this PR is fine. Moving it out of Draft state would seem like a good start.

@GallLeo
Copy link
Contributor

GallLeo commented Feb 2, 2024

MAP-LIB meeting 2024-02-02:
We already agreed to not open up to all Modelica 3.6 features.
But, we bumped the version in order to e.g. use figure annotations (as long as no tool vendor complains).
Therefore, syntax check should also be bumped to Modelica 3.6.

@GallLeo GallLeo marked this pull request as ready for review February 2, 2024 09:54
@GallLeo GallLeo added this to the MSL4.1.0 milestone Feb 2, 2024
@GallLeo
Copy link
Contributor

GallLeo commented Feb 2, 2024

@Harisankar-Allimangalath Please back-port to release-branch v4.1.0

@beutlich
Copy link
Member Author

beutlich commented Feb 2, 2024

MAP-LIB meeting 2024-02-02: We already agreed to not open up to all Modelica 3.6 features. But, we bumped the version in order to e.g. use figure annotations (as long as no tool vendor complains). Therefore, syntax check should also be bumped to Modelica 3.6.

Sorry, I cannot follow. Enabling MoLang 3.6 syntax also opens all MoLang 3.6 features (as the new break keyword).

@maltelenz
Copy link
Contributor

That was what was decided last year, that we would open up for 3.6 generally. However, for each individual pull request that uses new features, tool vendors can object that they are not ready.

@Harisankar-Allimangalath
Copy link
Contributor

@maltelenz @beutlich How should this be proceeded ?

@maltelenz maltelenz merged commit bc5c458 into modelica:master Feb 5, 2024
2 checks passed
@maltelenz
Copy link
Contributor

@Harisankar-Allimangalath please create a pull request to backport to 4.1.0 maint branch.

@beutlich beutlich deleted the update-moparser-3.6 branch February 5, 2024 10:28
@beutlich
Copy link
Member Author

beutlich commented Feb 5, 2024

@Harisankar-Allimangalath please create a pull request to backport to 4.1.0 maint branch.

Really? I hope that we do not do this.

@maltelenz
Copy link
Contributor

@Harisankar-Allimangalath please create a pull request to backport to 4.1.0 maint branch.

Really? I hope that we do not do this.

Could you explain why not?

@beutlich
Copy link
Member Author

beutlich commented Feb 5, 2024

Because we do not want to allow new syntax features in the branched-off maint branch, do we?

@maltelenz
Copy link
Contributor

The decision by MAP-LIB was to use Modelica 3.6 in 4.1.0, so yes, technically we do want to allow exactly that.

In practice, tool vendors will object if a pull request using break appears on the maint branch for 4.1.0.

@Harisankar-Allimangalath
Copy link
Contributor

Harisankar-Allimangalath commented Feb 6, 2024

@AHaumer should I backport this? This delays the tool vendor testing.

@beutlich
Copy link
Member Author

beutlich commented Feb 6, 2024

This delays the tool vendor testing.

I am afraid I cannot follow. This a CI feature and tool vendors can test with or w/o it already.

@Harisankar-Allimangalath
Copy link
Contributor

Harisankar-Allimangalath commented Feb 6, 2024

@beutlich ok , so once #4299 and #4298 is merged I can tagout the beta tag right ?

@maltelenz
Copy link
Contributor

@Harisankar-Allimangalath The real blocker for tool vendor testing (at least ours) is that we don't have new reference results.

@beutlich beutlich removed this from the MSL4.1.0 milestone Feb 7, 2024
@beutlich
Copy link
Member Author

beutlich commented Feb 7, 2024

@beutlich ok , so once #4299 and #4298 is merged I can tagout the beta tag right ?

IMO it is not yet there. The changes since alpha.1 are hardly relevant so far whereas the actual open issues haven't be addressed yet.

@beutlich
Copy link
Member Author

The real blocker for tool vendor testing (at least ours) is that we don't have new reference results.

Unblocking -> Results by Dymola 2024x of MSL v4.1.0-beta.1 (with serveral necessary changes) are here.

@maltelenz
Copy link
Contributor

The real blocker for tool vendor testing (at least ours) is that we don't have new reference results.

Unblocking -> Results by Dymola 2024x of MSL v4.1.0-beta.1 (with serveral necessary changes) are here.

While the effort is appreciated, I was hoping for these things before looking at correctness failures for System Modeler:

  • Only updating the reference results for models where we have good reason (model changes)
  • At least a categorization of the current failures in the regression testing with Dymola, which I think @GallLeo and @casella started on?

@beutlich
Copy link
Member Author

While the effort is appreciated, I was hoping for these things before looking at correctness failures for System Modeler:

* Only updating the reference results for models where we have good reason (model changes)

Uff, not sure if this can be tackled at all while also updating the simulation tool.

* At least a categorization of the current failures in the regression testing with Dymola, which I think @GallLeo and @casella started on?

Well, #4296 only touched four models and there was no vibrant discussion going on which I would have expected before releasing a beta release.

@maltelenz
Copy link
Contributor

Well, #4296 only touched four models and there was no vibrant discussion going on which I would have expected before releasing a beta release.

Right. I don't know what the intention is with the beta, but if it is a signal that it is ready for tool vendor testing, it's clearly not the case yet :(

@beutlich
Copy link
Member Author

beutlich commented Feb 12, 2024

Yes, intention of the beta is to initiate tool vendor testing. However with reference results being avialble only now, the process is already broken. Still, you should not be blocked anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Issue that addresses continuous integration requires Modelica 3.6 Issue that requires Modelica Language Specification 3.6
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants