Opened 6 years ago
Closed 5 years ago
#5490 closed enhancement (fixed)
Make the NF the default choice for code generation with automatic fallback to OF
Reported by: | Francesco Casella | Owned by: | Adeel Asghar |
---|---|---|---|
Priority: | blocker | Milestone: | 1.14.0 |
Component: | OMEdit | Version: | |
Keywords: | Cc: | Per Östlund, Adrian Pop, massimo ceraolo, Andrea Bartolini, Rüdiger Franke |
Description
During the Board meeting of 17-05-2019 it was decided to make the new frontend the default option for code generation in 1.14.0, so that people start using it and either get the benefit of improved performance and coverage, or help finding remaining bugs. On the other hand, we need to make sure we don't have users complaining that models that used to work in 1.13.2 now fail with 1.14.0, so it was also decided to have some kind of automatic OF-fallback option.
Analysis of the results of the testsuite using the OF and NF as of today reveals that:
- the NF coverage of the MSL 3.2.3 is 100%, and the remaining issues with models that fail to simulate or prod uce wrong results against the references are due to backend/runtime issues, see #5288
- the overall simulation coverage with the NF is only slightly lower than the one with the NF (7375/8939 vs.7658/8939), mostly because of #5204 - as soon as #5204 is fixed, the NF coverage is very likely going to take over
- after the bugfixes in the last few months, there are no known cases whereby the NF successfully leads to simulation with wrong results, while the OF leads to simulation with correct results, while there are known cases of the contrary occurrence, e.g. #2858.
Given these facts, I would suggest to implement the Board Decision in OMEdit in the following way:
- We change the text of the option "Enable experimental new instantiation phase" to "Enable new frontend for code generation", which reflects more accurately what happens, and refers to "new frontend", which is the buzzword we've used consistently in the last three years
- We make that true by default
- If the code generation fails during frontend, backend, C code generation, and C code compilation, a warning is issued like "Code generation using the new(old) frontend fails. Retrying with the old(new) one. You may change the preferred frontend option in Tools | Options | Simulation | Enable new frontend for code generation". Then, the code generation is attempted using the other option
As I see it, this set-up should improves the user experience in terms of increased compilation speed and reduced number of failures, while avoiding cases in which models that were previously simulating correctly in 1.13.2 will cease to do so in 1.14.0.
Regarding the scripting environment, I would instead keep it like it is, only make -d=newInst the default and explain in the release notes that in case this generate trouble, you can set the compiler option -d=nonewInst and use the old one. People using scripting have a deeper technical understanding, so this should be ok. I would avoid the automatic retry option, because this could cause weird behaviour in complex script-based applications.
Change History (27)
comment:1 by , 6 years ago
Cc: | added |
---|
follow-up: 4 comment:2 by , 6 years ago
follow-ups: 6 8 comment:3 by , 6 years ago
In my opinion it would've been better to release 1.14.0 first, then immediately switch the nightly builds to using the NF by default as preparation for 2.0.0.
But if we're going to change the option in OMEdit, maybe it would be better to reverse it so that it says e.g. "Use old frontend for code generation.". I can't really say why, but that feels more logical to me. But I don't really know how OMEdit handles settings, so that might influence the decision too.
follow-up: 5 comment:4 by , 6 years ago
Replying to adrpo:
The only problem I see with this is: what happens if there is an actual error in the user model? Should we re-try with the OF? The tool doesn't really know if is a bug in NF or an actual user error.
I guess OMEdit should store any errors from the NF, and if both the NF and the OF fails display those, otherwise just discard them. But falling back to the OF will mean that NF issues will likely not be reported, so this might be problematic if we remove that fallback later.
follow-up: 7 comment:5 by , 6 years ago
Replying to perost:
Replying to adrpo:
The only problem I see with this is: what happens if there is an actual error in the user model? Should we re-try with the OF? The tool doesn't really know if is a bug in NF or an actual user error.
I guess OMEdit should store any errors from the NF, and if both the NF and the OF fails display those, otherwise just discard them. But falling back to the OF will mean that NF issues will likely not be reported, so this might be problematic if we remove that fallback later.
Well since OMEdit uses translateModel
so it can first run it with NF and if there are any errors just report them and then show the warning message suggested by Francesco and then try with the OF and show the errors if any. The only drawback is that we are going to translate the model twice in case of actual user error.
comment:6 by , 6 years ago
Replying to perost:
In my opinion it would've been better to release 1.14.0 first, then immediately switch the nightly builds to using the NF by default as preparation for 2.0.0.
But if we're going to change the option in OMEdit, maybe it would be better to reverse it so that it says e.g. "Use old frontend for code generation.". I can't really say why, but that feels more logical to me. But I don't really know how OMEdit handles settings, so that might influence the decision too.
I guess this is fine. Some people might have already saved the option to use the NF so I just need to reverse the saved option.
comment:7 by , 6 years ago
Replying to adeas31:
Replying to perost:
Replying to adrpo:
The only problem I see with this is: what happens if there is an actual error in the user model? Should we re-try with the OF? The tool doesn't really know if is a bug in NF or an actual user error.
I guess OMEdit should store any errors from the NF, and if both the NF and the OF fails display those, otherwise just discard them. But falling back to the OF will mean that NF issues will likely not be reported, so this might be problematic if we remove that fallback later.
Well since OMEdit uses
translateModel
so it can first run it with NF and if there are any errors just report them and then show the warning message suggested by Francesco and then try with the OF and show the errors if any. The only drawback is that we are going to translate the model twice in case of actual user error.
One issue with that is that OMEdit might show errors from the NF and then the model works anyway, which might be confusing for the users. But I don't think there's a perfect solution for this, so we will have to compromise in some way.
comment:8 by , 6 years ago
Cc: | added |
---|
Replying to perost:
In my opinion it would've been better to release 1.14.0 first, then immediately switch the nightly builds to using the NF by default as preparation for 2.0.0.
This has been the plan for a long time, assuming the new API would come of age much earlier than the NF. But now the NF is actually quite good, so I think that pushing the users to actually use it is a good idea. Otherwise, most users won't bother with the NF until 2.0.0, that is to say, next year. That's probably too late.
Replying to adrpo:
The only problem I see with this is: what happens if there is an actual error in the user model? Should we re-try with the OF? The tool doesn't really know if is a bug in NF or an actual user error.
You are absolutely right, I overlooked this obvious issue. We have to deal with this, while being very careful not to annoy users.
A situation whereby most of the times the compilation is performed twice is obviously unacceptable. I guess we have to introduce some degree of interaction. Here is one proposal.
1)
Run the code generation (i.e. translateModel
) with the currently selected front-end (NF by default)
2)
In case of errors during any phase of the code generation, show a pop up window with this message:
The code generation process failed, see the Message Browser and the Simulation Output | Compilation windows.
Most likely this is due to some issues in the Modelica source code, but it could also be due to some Open Modelica compiler issue. In this case, you may re-try the code generation with the old (new) frontend, see also Tools | Options | Simulation | Enable new frontend.
Then have buttons with these three options
- Try with old (new) frontend once
- Switch to old (new) frontend permanently
- Cancel
plus a checkbox "Don't ask me anymore".
What do you think?
comment:9 by , 6 years ago
As an additional comment, if the error messages contain words as "Internal error", then we can conclude with high certainty the the problem is not with the Modelica code but only with the compiler, and use a more appropriate text for the message window.
follow-up: 11 comment:10 by , 6 years ago
Then have buttons with these three options
Try with old (new) frontend once
Switch to old (new) frontend permanently
Cancel
plus a checkbox "Don't ask me anymore".
Try with old (new) frontend once and Don't ask me anymore doesn't fit well. I suggest that we always pop-up a dialog whenever the translateModel
fails just rely on the result of it instead of parsing Internal error
from the messages. This is anyway a temporary solution until we shift completely to NF.
comment:11 by , 6 years ago
Cc: | added |
---|
Replying to adeas31:
Then have buttons with these three options
Try with old (new) frontend once
Switch to old (new) frontend permanently
Cancel
plus a checkbox "Don't ask me anymore".
Try with old (new) frontend once and Don't ask me anymore doesn't fit well.
Why not? Try with the old/new frontend once is to see what happens with the other FE. Don't ask me anymore is instead meant to avoid harassing the users indefinitely with this popup, every time something goes wrong with the code generation. Maybe we should just have "Don't ask me anymore for this session".
Maybe we should hear other opinions, I am adding Ruediger, Massimo and Andrea in cc:
I suggest that we always pop-up a dialog whenever the
translateModel
fails just rely on the result of it instead of parsingInternal error
from the messages. This is anyway a temporary solution until we shift completely to NF.
Maybe this would be a bit intrusive in the long term, see above.
follow-up: 13 comment:12 by , 6 years ago
If I understand well (from the last sentence of comment 10), there will be a time in which the only option available will be NF (and OF will not be accessible anymore). In this case, for the limited period (one year?) in which both front-ends are available, all the proposals in this ticket (I mean in comments 8 and 10, even if I don't understand completely the latter) are ok for me.
If, on the contrary, I'm mistaken on this, I think that when the NF is standard and considered by large the best option, and OF kept only for special cases, the question to users on what to do should disappear.
In fact, in that case expert users could easily find the way to activate OF, when needed, without any question. On the other hand I think we must allow Students and new or occasional users not to even understand what "front end" means.
comment:13 by , 6 years ago
Replying to ceraolo:
If I understand well (from the last sentence of comment 10), there will be a time in which the only option available will be NF (and OF will not be accessible anymore).
Yes, absolutely. The OF has a large number of issues that cannot be resolved, and is no longer maintained. It is going to be retired, as soon as the NF can handle everything the OF can, which is expected to happen before the end of 2019.
In this case, for the limited period (one year?) in which both front-ends are available, all the proposals in this ticket (I mean in comments 8 and 10, even if I don't understand completely the latter) are ok for me.
In fact, in that case expert users could easily find the way to activate OF, when needed, without any question. On the other hand I think we must allow Students and new or occasional users not to even understand what "front end" means.
True. This is basically the idea behind making the NF the default in 1.14.0. However, for some months yet there will be some cases of models that happily worked with the OF in 1.13.2 and may break with the NF in 1.14.0. We need to make sure that non-experts can easily manage this case, less they get angry at the OSMC. Hence, the pop-up window with the buttons.
follow-up: 15 comment:14 by , 5 years ago
We could have a flag --frontend=1|2|2WithFallback
and make the fallback only trigger if the frontend succeeded but the backend failed. If the new frontend fails in the frontend, it is most likely a user error anyway... But if you have an error in the backend you will get huge error-messages twice and it will take a long time to translate.
I would prefer if there was perhaps a button Simulate with old frontend
or some GUI support to ask the user if translation failed with the new frontend (that could be disabled). Currently it is annoying to change between the frontends in OMEdit.
comment:15 by , 5 years ago
Replying to sjoelund.se:
I would prefer if there was perhaps a button
Simulate with old frontend
or some GUI support to ask the user if translation failed with the new frontend (that could be disabled). Currently it is annoying to change between the frontends in OMEdit.
That's exactly the point of the proposal in comment:8, which aims at guiding the user through a choice that he/she may otherwise not understand, and make it possible with one mouse click. What do you think about it?
Otherwise, it's three mouse clicks: Simulation Setup | Translation Flags | Enable new front-end
follow-up: 17 comment:16 by , 5 years ago
comment:8 suggested mostly to have that behavior of trying with one frontend and switch. I think it would be nicer to only have a button to simulate with old (or new depending on your preferences in the settings) as a button similar to simulate with debugger, etc today.
For simulation setup have it on the General screen. But we should move around some other things. That setup tab is very tall as it is and I guess Interactive simulation could have its own tab. The dassl/IDA options could be to the right of the dassl settings. I need to go to my portrait mode screen to be able to see all the options in the general tab today...
One alternative would be to not use a checkbox but have boxes for old/new/cancel instead of ok/cancel.
comment:17 by , 5 years ago
Replying to sjoelund.se:
comment:8 suggested mostly to have that behavior of trying with one frontend and switch. I think it would be nicer to only have a button to simulate with old (or new depending on your preferences in the settings) as a button similar to simulate with debugger, etc today.
Sounds good, unless you want to simulate with old and debugger
For simulation setup have it on the General screen. But we should move around some other things. That setup tab is very tall as it is and I guess Interactive simulation could have its own tab. The dassl/IDA options could be to the right of the dassl settings. I need to go to my portrait mode screen to be able to see all the options in the general tab today...
Maybe we should reconsider the layout of those settings for 2.0.0
One alternative would be to not use a checkbox but have boxes for old/new/cancel instead of ok/cancel.
Sounds good.
comment:18 by , 5 years ago
The 1.14.0 beta release date is approaching. Together with faster GUI response and replaceable support, this may be the most consequential change in the end-user experience.
Do we have a consensus on how we do this? I have no strong opinions on the actual impelementation, but the requirements for me are clear:
- use the NF by default
- allow to switch to OF with one mouse click if potentially useful
- communicate clearly to the end user what is going on
- avoid potentially annoying pop-ups popping up all the time.
This was the rationale of my proposal in comment:8. Any other proposal that fits the requirements is fine for me, but we should reach an agreement very soon.
follow-up: 20 comment:19 by , 5 years ago
@casella can you provide a simple model that fails with the NF and works with the OF?
comment:20 by , 5 years ago
Replying to adeas31:
@casella can you provide a simple model that fails with the NF and works with the OF?
Here's one such model:
model M model A parameter Integer n; Real x[n]; end A; A a[2](n = {1, 2}); end M;
follow-up: 22 comment:21 by , 5 years ago
Thank you @perost, I would have had a hard time finding such a small one :)
BTW, should we perhaps go for a plan B and scalarize these cases, until you find a vectorized solution?
comment:22 by , 5 years ago
Replying to casella:
BTW, should we perhaps go for a plan B and scalarize these cases, until you find a vectorized solution?
That might be about as hard as just handling them correctly non-scalarized, if not harder. The whole NF is designed with the assumption that nothing is scalarized until the flattening phase.
follow-up: 24 comment:23 by , 5 years ago
Are we going to get something to work with parameter arrays and arrays of records before the end of 2019?
follow-up: 25 comment:24 by , 5 years ago
Replying to casella:
Are we going to get something to work with parameter arrays and arrays of records before the end of 2019?
The main priority is improving library support, so it depends on how much it's used in libraries. I think this particular pattern is very rare, and in my opinion borderline illegal (since it creates an array where the elements have different types). The NF has a special case for when the dimensions are all the same, since I've seen some models that do that (in Buildings I think). But it'll probably get implemented some time, it's just a matter of priority.
comment:25 by , 5 years ago
Replying to perost:
Replying to casella:
Are we going to get something to work with parameter arrays and arrays of records before the end of 2019?
The main priority is improving library support
That's what I was referring to. The coverage curves produced by Jenkins have been quite flat in the last three months :)
comment:27 by , 5 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
I've tested this for a while, and it looks good and very practical. Feel free to reopen if issues pop up.
The only problem I see with this is: what happens if there is an actual error in the user model? Should we re-try with the OF? The tool doesn't really know if is a bug in NF or an actual user error.