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

Instance-based mechanism to handle parameters, replaceable classes, conditional connectors, and parameter-dependent dialogs #8425

Open
casella opened this issue Jan 15, 2022 · 40 comments
Assignees
Labels
COMP/GUI/OMEdit Issue and pull request related to OMEdit COMP/OMC/Interactive Environment
Milestone

Comments

@casella
Copy link
Contributor

casella commented Jan 15, 2022

The current mechanism that OMEdit uses for parameter editing and to display component icons in diagrams has OMEdit figure out what are the relevant classes ad inquire their definitions in the used libraries. In this way, local modifications, inheritance, etc. need to be handled ad-hoc by OMEdit, which is clumsy and unnecessarily complicated. See discussion here.

We need to transition to a new mechanism, whereby OMEdit enquires an instance of the model, relying on the new front end to carry out all the complicated work. The NF is very fast, so this is now doable. Additionally, a special "fast instantiation" should be implemented, that only flattens constants, parameters (with their binding equations) and annotations.

This development will be carried out in Q1-Q2 2022.

Doing this will address the root cause of numerous issues that have popped up over the years, and have remained unresolved to date, because fixing the old mechanism is really not an option. These issues are collected here for reference:

While designing this new mechanism, we should also keep in mind that we need to support hierarchical editing of parameters and replaceable stuff in models, a.k.a. "Show component" in Dymola:

This feature also requires to instantiate models and work on the instance to change modifiers

@casella casella added COMP/GUI/OMEdit Issue and pull request related to OMEdit COMP/OMC/Interactive Environment labels Jan 15, 2022
@casella casella added this to the 1.20.0 milestone Jan 15, 2022
@casella
Copy link
Contributor Author

casella commented Jan 15, 2022

@AHaumer, @max-privato, @sbaetzing, @niklwors, @dietmarw, @AndreaBartolini, please follow this ticket to see the progress, instead of #2081, which is only a part of the story.

@adrpo
Copy link
Member

adrpo commented Feb 23, 2022

Design

When queried, OMC will return a JSON of the instantiated tree including:

  • parameter, constants and replaceable components
  • replaceable classes
  • unevaluated graphical and dialog annotations
  • inherited class hierarchy (?)

Retrieving a model instance: getModelInstance(Path.To.Model): JSON

Calling this API will return the instance tree of the model.
Possible optimization:

  • do partial instantiation of only needed things (no equ/alg sections)
  • request just part of the tree via some filters
  • compress the data

Sending back modifications: setModelInstance(JSON): Boolean

Calling this API will apply the modifications from the JSON to the top level Modelica model in the AST.
Possible optimization:

  • send only the changes, not the entire model with the changes back

Evaluating expressions on a model instance: evaluateExpressionOnModelInstance(Expression, Path.To.Model): JSON

Calling this API will return the evaluated expression.
Possible optimization:

  • return the both the unevaluated and evaluated expression as part of the instance tree

GUI <-> OMC communication

The usual workflow:

  1. GUI: asks for an model instance via getModelInstance
  2. OMC: responds with a JSON tree
  3. GUI: parses the JSON and populates its internal structures
  4. GUI: generates the Diagrams and Dialogs
  5. GUI: The user changes data in Dialogs or Diagrams
    • if possible (no function calls) GUI evaluates locally the expressions needed for Diagrams and Dialogs dependent of the changed data
    • if not possible (there are function calls) GUI asks OMC to evaluate the expressions needed on on the model instance
  6. GUI: go to step 4 and maybe also do in parallel steps 6-7
  7. GUI: sends the changes back to OMC as a JSON file with only the differences
  8. OMC: applies the changes on the top level Modelica model in the internal AST

Notes

Some things to think about:

  • how to encode Modelica expressions: JSON tree (quite large) or just string (compact) which we can parse with OMParser?
  • how should we provide the choices (in choicesAllMatching/choice) and their respective trees so that they can be edited?

@adrpo
Copy link
Member

adrpo commented Feb 23, 2022

@adeas31, @perost, @casella: please read and give feedback on the Design above.

@sjoelund
Copy link
Member

There is JSON.mo file that could be used and passed to OMEdit that it could possibly transform to QVariant or work with directly. I would avoid passing large strings

@perost
Copy link
Member

perost commented Feb 23, 2022

There is JSON.mo file that could be used and passed to OMEdit that it could possibly transform to QVariant or work with directly. I would avoid passing large strings

Yeah, I'm using the existing JSON structures, but dumping it to a string to pass to OMEdit. We'll see how well that works, but dumping the JSON to a string is trivial anyway and something we likely need for OMWebEdit anyway.

@sjoelund
Copy link
Member

Sure, dumping to string is fine for the scripting API. But for OMEdit there's a potentially faster way. And you'd get the same data types if you parse JSON to QVariant so you could just add the shortcut later and prototype this way.

@adeas31
Copy link
Member

adeas31 commented Mar 1, 2022

inherited class hierarchy (?)

This is needed in order to draw the complete elements.

request just part of the tree via some filters

We don't need to worry about that part. Since in a normal work flow OMEdit will only ask the JSON once in the beginning and then send back the edited parts to OMC. The only other case when the whole JSON is needed is when user edits the Modelica text manually.

Evaluating expressions on a model instance:

I didn't get that part. Maybe you can elaborate more with some examples.

@adeas31
Copy link
Member

adeas31 commented Mar 1, 2022

There is JSON.mo file that could be used and passed to OMEdit that it could possibly transform to QVariant or work with directly. I would avoid passing large strings

Even if we transform to Qt data structures we still have to somehow pass them to OMEdit.

@sjoelund
Copy link
Member

sjoelund commented Mar 1, 2022

By MetaModelica void* :)

@casella
Copy link
Contributor Author

casella commented Mar 1, 2022

I have one comment, based on our past discussion. When editing a model with modifiers, OMC should not only return the actually modified values, but also the default values those parameters would have, in case one deletes the modifed value and wants to see the underlying default. I'm not sure if this feature is already included in @adrpo's proposal.

@adrpo
Copy link
Member

adrpo commented Mar 2, 2022

Evaluating expressions on a model instance:

I didn't get that part. Maybe you can elaborate more with some examples.

For example, have this model:

model Test
  parameter Boolean a = false;
  parameter Real b annotation(Dialog(enable=someModelicaFunction(a));
end Test;

OMEdit would get some JSON that could look like:

{
   "id": "someUniqueNumber",
   "name": "Test",
   "elements": [
        {
           "name": "a",
           "value": "false"
        },
        {
           "name":  "b",
           "annotation": "Dialog(enable=someModelicaFunction(a)"
        }
   ]
} 

How would OMEdit evaluate expression: someModelicaFunction(a)?
It would not be able to, it needs to send it to OMC for evaluation:
evaluateExpressionOnModelInstance("someModelicaFunction(a)", "Test")
would get you something like:

{
   "expression": "someModelicaFunction(a)",
   "path": "Test",
   "result": "true"
} 

Of course we could just maybe assign an unique ID for each JSON we send OMC->OMEdit and then refer to that:
evaluateExpressionOnModelInstance("someModelicaFunction(a)", "someUniqueNumber")
would get you:

{
   "expression": "someModelicaFunction(a)",
   "id": "someUniqueNumber",
   "result": "true"
} 

@adrpo
Copy link
Member

adrpo commented Mar 2, 2022

I have one comment, based on our past discussion. When editing a model with modifiers, OMC should not only return the actually modified values, but also the default values those parameters would have, in case one deletes the modified value and wants to see the underlying default. I'm not sure if this feature is already included in @adrpo's proposal.

This is not part of my proposal yet. But it should be easy to have.

model Test
  model M
     parameter Real a=10;
   end M;
   
   M m(a=20);
end Test;

Then one could have a JSON looking like:

{
     "name": "Test"
     "elements": [
       {
          "name": "M",
          "elements": [ 
            {
              "name": "m",
               "type": "Real",
               "value": "10"
             }
           ]
       },
       {
          "name": "m"
          "type": "M",
          "modifiers": [
            {
               "name": "a",
               "original": "10", // we could also have a modifier chain if there are several levels of modifications [10, 20]
               "value": "20",
               "each": "none" // to finally support each in the dialogs!
            }
          ]
       }
   ]     
}

@adrpo
Copy link
Member

adrpo commented Mar 2, 2022

There is JSON.mo file that could be used and passed to OMEdit that it could possibly transform to QVariant or work with directly. I would avoid passing large strings

Even if we transform to Qt data structures we still have to somehow pass them to OMEdit.

What Martin says is that we could write a walker that directly walks MetaModelica structures and gives you a Qt tree in OMEdit. That might be problematic with the instance tree as it has cycles.

@sjoelund
Copy link
Member

sjoelund commented Mar 2, 2022

If you have a JSON.mo structure with cycles and try to convert to string, you'll have the same problem.

@adrpo
Copy link
Member

adrpo commented Mar 2, 2022

Maybe we should also make an exercise and see how each existing interactive API would be supported with the new one.

@perost
Copy link
Member

perost commented Mar 10, 2022

I've now opened #8676 with a first draft that implements getModelInstance, but there are a few issues that we need to solve:

  • Classes instanced by getModelInstance are currently not cached, not clear if they need to be. We'll need an instance to evaluate expressions passed from OMEdit, and having to instantiate the class all over again for every expression would not be performant. On the other hand we can't just cache every instance or we'll run out of memory quickly, but just caching the last one is maybe not good either since OMEdit can have multiple classes open.
    adrpo: maybe cache using LRU? NFApi.mo does a lot of caching.
  • Passing modifiers back in the form of JSON is a bit problematic since there's no connection between the Absyn and the NF (the NF uses SCode), so we somehow need to use the NF information to figure out where to put the modifiers in the Absyn (directly on local elements or on the extends clauses for inherited elements).
    adrpo: one needs to take the modifiers and apply them (maybe deleting the old ones or merging) on the top level class components (or classes).
  • Dimensions are evaluated during the typing, so we don't have the original instantiated dimensions easily available.
  • Unclear how to handle dimensions since they can come from both a component's declaration as well as the component's type. Both are needed to determine if something is an array, but only the declaration dimensions can be changed when editing a component.
    adrpo: a bit unsure if it matters here as we would not check if the modifier agrees with the type in OMEdit, needs to be done in OMC after is applied; maybe to provide some nice tools to input matrixes or arrays in Qt.
  • Some expressions are evaluated during typing, notably fill (and by extension ones and zeros). It would be preferable to not evaluate fill so early but it's needed right now to avoid type issues, but maybe it's enough to just evaluate the size of the fill-call?
    adrpo: for sure we would need to avoid evaluating expressions (or keep all the steps if possible)
  • Evaluating component references overwrites the binding of the component (so we don't have to evaluate it multiple times), so for e.g. component references used to determine dimension we don't have easy access to the instantiated original binding.
    adrpo: for sure one could keep the history
  • Component annotations are not instantiated. adrpo added a field to components for holding the instantiated annotation as part of the work on the nfAPI, but it's not actually used at the moment.
    adrpo: yes that needs to be expanded and actually implemented
  • For constants and structural parameters we return both the evaluated and non-evaluated binding expressions, unclear if we also should do so with non-structural parameters (those that can be evaluated that is).
    adrpo: we should do it if they appears in annotations i guess or not if we can detect that OMEdit can evaluate it
  • The component prefixes returned include prefixes inherited by the component from its parent, do we want that or only the prefixes from the component declaration itself?
    adrpo: let's have it (we can display it as grey or some other color for the inherited ones
  • Prefixes are currently dumped as an array with the same format as getComponentInfo uses since it's reasonably compact, but maybe using a JSON object would be better and only add the prefixes which are different than the default prefixes.
    adrpo: not sure yet, let's see how do we need to use them
  • Extends nodes are removed already during the second phase of the instantiation. Do we need any information about inherited elements?
    adrpo: maybe we need to know where they come from (to at least help the user understand where this parameter comes from)
  • Need to somehow handle components that couldn't be instantiated/typed. Might be hard to deal with the consequences, like dealing with invalid components during constant evaluation.
    adrpo: this should not be that complicated, i guess mark them as deleted components and if they are used, just report error
  • Equations/algorithms are still instantiated/typed at the moment. Not sure if we actually want to avoid that completely, we have some API functions for equations/algorithm/connections but maybe none that needs them to be instantiated?
    adrpo: i think we could avoid them completely but we need their annotations for sure at least for connects
  • Replaceable classes was mentioned, do we just need a list of the names of the replaceable classes declared in an instance or something more involved?
    adrpo: we need all the parameters and replaceable classes/components from them so we can edit those in the dialogs
  • Do we want some information about imports?
    adrpo: not sure here, for now let's skip it
  • Deleted components are normally not typed since they'll just be removed anyway. We might not need to type them in order to dump them since they're not allowed to be used in expressions, and typing them can be problematic since they're not always valid if they're meant to be removed. But the dumping will need to take into account that some things are not typed in that case.

And probably a few more I've forgotten.

@perost
Copy link
Member

perost commented Mar 11, 2022

#8683 fixes the issue with EngineV6 that adrpo mentioned in #8676. BatchPlant_StandardWater still doesn't work though, for some reason we get some untyped type attributes in it.

@perost
Copy link
Member

perost commented Mar 11, 2022

The issue with BatchPlant_StandardWater is that we don't type deleted components since that they'll just be removed anyway, so we get issues when we try to dump them. We might not actually need to type those since we should never need to evaluate a conditional component, but I'll have to look into that.

@adrpo
Copy link
Member

adrpo commented Mar 11, 2022

But i guess OMEdit doesn't need to see them, so we might as well not dump any of them in the JSON.

@perost
Copy link
Member

perost commented Mar 11, 2022

But i guess OMEdit doesn't need to see them, so we might as well not dump any of them in the JSON.

I thought being able to handle conditional connectors was a big part of what we're trying to fix?

@casella casella modified the milestones: 1.20.0, 1.21.0 Oct 10, 2022
@niklwors
Copy link
Contributor

@perost With the current version from today OMEdit hangs at startup unfortunately.

@niklwors
Copy link
Contributor

I was able to start OMEdit with today's version. However, it is not possible to expand a folder in the library browser because it has been trying for half an hour to exapnd the first subfolder of the Rexroth_Valves.ProportionalValves package. So it is unfortunately not possible to test and use the OMedit with the new API.

@casella
Copy link
Contributor Author

casella commented Oct 12, 2022

@niklwors we need to check what is going wrong there. In the meantime, maybe you can double-check with other libraries?

@niklwors
Copy link
Contributor

@casella The problem occurs with all libraries.

@perost
Copy link
Member

perost commented Oct 13, 2022

@casella The problem occurs with all libraries.

That's really odd, and also that it was hanging at startup previously. We haven't actually changed anything that should affect that for the last couple of days, and I can't reproduce the issue myself.

@niklwors
Copy link
Contributor

For example I opened in the library browser the folder Rexroth_Pumps_Test.AxialPistonUnits.Components and it hangs. When I killed the omedit.exe process after 30 minutes the last command in omeditcommands.mos is getNamedAnnotation(Rexroth_Generic_Test.Auxiliaries.UnitConversion.TorqueConversionFromSI_TC01`, versionBuild); getErrorString();
It is the 36408 call.
My colleague tested it on his computer an he has the same issue.

@adeas31
Copy link
Member

adeas31 commented Oct 13, 2022

The logs are not flushed when the process is killed by force.

I think it is spinning when getModelInstance(Rexroth_Pumps_Test.AxialPistonUnits.Components.A4VSO40_10_TC01) is called. Here is a simple script to test,

loadFile("Rexroth_Pumps_Test/package.mo");
getErrorString();
getModelInstance(Rexroth_Pumps_Test.AxialPistonUnits.Components.A4VSO40_10_TC01);
getErrorString();

@adrpo we really need a job that tests the getModelInstance api.

@adrpo
Copy link
Member

adrpo commented Oct 17, 2022

I will try to do something about it this week but I'm currently swamped.

@perost
Copy link
Member

perost commented Oct 23, 2022

I think it is spinning when getModelInstance(Rexroth_Pumps_Test.AxialPistonUnits.Components.A4VSO40_10_TC01) is called.

Fixed in #9586 (which also fixes #9533).

@casella casella moved this from To do to In progress in Improvements for BR Oct 23, 2022
@niklwors
Copy link
Contributor

I think it is spinning when getModelInstance(Rexroth_Pumps_Test.AxialPistonUnits.Components.A4VSO40_10_TC01) is called.

Fixed in #9586 (which also fixes #9533).

I have tested it with the current OMEdit version but the problem is still there.

@perost
Copy link
Member

perost commented Oct 24, 2022

I think it is spinning when getModelInstance(Rexroth_Pumps_Test.AxialPistonUnits.Components.A4VSO40_10_TC01) is called.

Fixed in #9586 (which also fixes #9533).

I have tested it with the current OMEdit version but the problem is still there.

The issue is fixed from what I can see, but now the issue is that it's instead extremely slow and eventually segfaults for me after a couple of minutes. It doesn't look like it actually runs out of memory, but I get a warning from the GC about repeated large allocations that might cause memory leaks or other issues. So I guess the segfault is due to the GC not being able to keep up.

I'm not entirely sure how OMEdit works, but I'm assuming that we call getModelInstance one every class in the package in order to get the icon annotation. That's just not going to work, it will be far too slow and memory expensive to be usable for real libraries, so we'll need to come up with some other solution for that.

Edit: I should perhaps add that it doesn't get stuck but actually progresses through a couple of the components before segfaulting, so it's not an issue with a specific class but rather just a lot of large classes in one package.

@adeas31
Copy link
Member

adeas31 commented Oct 24, 2022

I'm not entirely sure how OMEdit works, but I'm assuming that we call getModelInstance one every class in the package in order to get the icon annotation. That's just not going to work, it will be far too slow and memory expensive to be usable for real libraries, so we'll need to come up with some other solution for that.

Yes, of course the getModelInstance used for drawing icons. It is just not possible to use 2 different implementations for drawing of icons and diagrams. Maybe we can have a minimalist version of getModelInstance for icons and when user actuallys wants to see the diagram then the full getModelInstance is used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
COMP/GUI/OMEdit Issue and pull request related to OMEdit COMP/OMC/Interactive Environment
Projects
Development

No branches or pull requests

6 participants