Enable the usage of a pre-built model executables by ModelicaSystemRunner

[joewa]

Related Issues

This pull request is a re-implementation of issue #165 because of a new library architecture @adeas31

Purpose

• Enable the usage of a pre-built model executable when instantiating ModelicaSystemRunner. This will gain some efficiency e.g. when only the parameters or the input data changes between different simulation runs.
• Avoid the unnecessary overhead of using an OMCSession (with ZeroMQ connection).
• Slim (conda) environments, suited for efficient "simulation only" workflows.
• Applying different variableFilter's (or overrides in general) when calling the pre-built model executables.

Approach

ModelicaSystemRunner will simulate pre-built models with customized parameters or inputs.

from OMPython.OMRunner import ModelicaSystemRunner
mod = ModelicaSystemRunner(modelname='BouncingBall', runpath='.')

The simulation can be executed (with a customized set of output variables) like this:

res_vars = ['h', 'v']
log_str += str(mod.simulate(
    resultfile=resfilepathname,
    overrideaux='variableFilter="'+'|'.join(list(res_vars))+'"'
))

The log_str contains the models output from stdout and stderr.

The Strategy: "Duck Typing" (ModelicaSystemRunner)

Instead of modifying all OMPython modules, I created a standalone class (ModelicaSystemRunner) that mimics the API of OMPython. It will run purely on standard Python libraries (xml.etree, subprocess), making it zero-dependency and perfect for efficient use in slim environments.

Idea for way forward

Create a module + class ModelicaSystemBase that has all functions that are common for ModelicaSystem, ModelicaSystemRunner and probably ModelicaSystemCmd but without dependency on OMCSession. This might reduce the need to duplicate code in all classes.

[joewa]

@adeas31

[syntron]

@joewa I think it would be possible to use os.PathLike instead of OMCPath to define ModelicaSystemBase - this should be quite simple and would allow to use it - as suggested - as a common base for ModelicaSystemRunner as well as ModelicaSystem

[joewa]

Did it. Thanks for your feedback @syntron

[syntron]

@joewa looks good to me! I'm thinking about a modification to ModelicaSystemDoE such that the ModelicaSystemRunner class could also be used ... would this be usefull? I'm also considering this as a simplification of the structure of the ModelicaSystemDoE class.

Some comments:

• file names: I would rename the files to ModelicaSystemBase.py and ModelicaSystemRunner.py - aligning it to the names of the classes they define
• within all of OMPython, I did quite some effort to replace usage of string based path handling by pathlib (pathlib.Path) - would this make sense for ModelicaSystemRunner?
• Some code of simulate() is now duplicated - is there a way to reuse existing code? I'm thinking mainly about the special handling for Windows ...
• The chdir() calls in simulate() are not needed at all; the work directory can be provided to subprocess.run() as argument - please check OMCSession.run_model_executable().
• perhaps it would be possible to use this function and OMCSessionRunData to further align the new class to the existing code? The only reason for function omc_run_data_update() not defined as staticmethod is, that OMCSessionDocker and OMCSessionWSL need information about the session; however, this could be changed for OMCSessionLocal such that the two function could be reused for ModelicaSystemRunner. This would reduce quite some complexity / imports!
• Topic: work directory - would it make sense to define a work direcory for ModelicaSystemRunner?
• The latest change in OM results in incompatibilities - starting with v1.27.0, the simulation parameters are no longer defined in the override file but as command line paramaters; please check PR Do not add simulation options to overrideFile #400 and PR Update override handling #402
• Some comments from unrelated sections of the code were removed / modified (mainly ModelicaSystem and ModelicaSystemDoE) - why?
• You replaced some/all(?) usage of | by Union - why? According to the Python doc, the short version is recommende (see: https://docs.python.org/3/library/typing.html#special-forms). And, as OMPython supports Python >= 3.10, it is available on all supported Python versions.
• Would linearize() / optimize() also be needed in the Runner class?
• I follow the recommendation to define all imports in 3 sections (python standard; 3rd party; current package) and alphabetical order within each section. See: https://coderivers.org/blog/python-package-import-order-convention/
• There is a print("Init done") at the end of ModelicaSystemRunner.__init__() - I think, this should be a log message.

[joewa]

Thanks for so much feedback @syntron

by this PR, I'd like to enable this new OMPython workflow that is now implemented in ModelicaSystemRunner. It will be probably useful. It needed such a big pull request to fit it into the new architecture. I tried avoiding to question or break the existing design of OMPython with OMCSession. But since I am currently not using all the features of OMPython that you mention, I am probably not the right person to push for evolution beyond ModelicaSystemRunner at the moment.

Since we agreed that the current approach is better than having ModelicaSystemRunner standalone, I think we should rather merge this pull request earlier than later as it is already a step forward. Then we will avoid more (possibly incompatible) pull requests piling up...

I find most of your recommendations plausible. Please consider them agreed if not mentioned below:

• Some code of simulate() is now duplicated - is there a way to reuse existing code? I'm thinking mainly about the special handling for Windows ...

I cannot test on Windows. So I preferred to keep whatever works.

• The chdir() calls in simulate() are not needed at all; the work directory can be provided to subprocess.run() as argument - please check OMCSession.run_model_executable().

The chdir() calls are useful in case the compiled binary is placed next to the .so/.dll that it needs to run in an environment where the user has no control over the path variable.

• perhaps it would be possible to use this function and OMCSessionRunData to further align the new class to the existing code? The only reason for function omc_run_data_update() not defined as staticmethod is, that OMCSessionDocker and OMCSessionWSL need information about the session; however, this could be changed for OMCSessionLocal such that the two function could be reused for ModelicaSystemRunner. This would reduce quite some complexity / imports!

I am not using OMCSession*, so I don't dare to change anything there.

• Topic: work directory - would it make sense to define a work direcory for ModelicaSystemRunner?

Probably yes. See also my chdir above.

• The latest change in OM results in incompatibilities - starting with v1.27.0, the simulation parameters are no longer defined in the override file but as command line paramaters; please check PR Do not add simulation options to overrideFile #400 and PR Update override handling #402

I am currently using omcompiler v1.25 that is in conda-forge. Could you please consider rebasing these PR's to this one?

• Some comments from unrelated sections of the code were removed / modified (mainly ModelicaSystem and ModelicaSystemDoE) - why?

I had to review all functions before moving it to ModelicaSystemBase. Some are now split between ModelicaSystemBase and ModelicaSystem. I might have removed some comments in case I found they are now deprecated or not adding information. Maybe I was a bit too enthusiastic when cleaning up.

• You replaced some/all(?) usage of | by Union - why? According to the Python doc, the short version is recommende (see: https://docs.python.org/3/library/typing.html#special-forms). And, as OMPython supports Python >= 3.10, it is available on all supported Python versions.

Some users are still at Python < 3.10, really. I'd prefer keeping the widest possible coverage rather than implementing the newest language features (unless we are not forced to do so).

• Would linearize() / optimize() also be needed in the Runner class?

Maybe. Could you draft a use case for this? (But please not within this PR)

So if you agree that this PR is a step forward, I would propose to merge it and handle more improvements in future PR's.
Of course, please let me know if you see any regression that must be fixed before.

[syntron]

@joewa I was thinking a lot about this topic and work on a (bigger) update in the last days - take this / PR #404 as a draft as it is complete mess regarding the commits on my side. Thus, only a large merged blob available at the moment. The main point was to get a runner class working which uses as many code from the original ModelicaSystem as possible. In several steps this got way out of hands => see PR # 404 for the code / RFC; please test & comment!

[joewa]

I will review and test and make pull requests directly to your branch https://github.com/syntron/OMPython/tree/syntron_RFC .
As soon as we are done, you can split it to several pull requests to https://github.com/OpenModelica/OMPython/tree/master
OK, @syntron ?