RFC - definition of a runner class using as much of the existing code as possible#404
RFC - definition of a runner class using as much of the existing code as possible#404syntron wants to merge 102 commits intoOpenModelica:masterfrom
Conversation
syntron
force-pushed
the
syntron_RFC
branch
5 times, most recently
from
6ef0900 to
0deda10
Compare
|
Some status update on this PR - the main points are:
The code defined here updates the internal structure of OMPython such that:
Furthermore, as OMCPath is working only for Python >= 3.12, another dimension exists. There is a dummy implementation of OMCPath for Python < 3.12 such that most of the functionality can also be used here. See PR #401 for a hint that even older Python version are used (Python 3.10 or even lower). These changes could be the baseline for a new documentation (see https://openmodelica.org/doc/OpenModelicaUsersGuide/latest/ompython.html). My idea was to include these changes as 4.x.0 and create a cut at the next release (or keep some kind of compatibility layer alive). (partly from the discussion in PR #387) |
|
The failed test is due to Windows - it runs well for v4.0.0 (see: https://github.com/syntron/OMPython/actions/runs/21842400755) and 4.x.x (see: https://github.com/syntron/OMPython/actions/runs/21841418638) |
|
This is a final version of my changes to get a 'runner' functionality to OMPython. The main parts:
This PR will be splitted into smaller steps. |
syntron
force-pushed
the
syntron_RFC
branch
2 times, most recently
from
be89d52 to
b20568b
Compare
OK; I will create small parts for review & merge; there are some changes on top to restructure the two main files |
Sorry I haven't got time to look into this yet. |
yes - this is the plan; the one RFC is way too big / too much changes to get an easy overview ... |
|
Summary of the changes in these RFC; the elements will be provided as single PRs. Basic concept / plan:
This means for some period of time, the new interface and the compatibility layer can exist in parallel (versions Steps for the implementation (see PRs below):
I tried to order the PRs based on the steps (A) to (H) but some will be out of order. If the PRs are merged, please try PRs based on this RFC (using the branch name in my fork syntron:OMPython as reference; I will use this comment to link the PRs step by step): (step A)
(step B)
(step C)
(step D)
(step E)
(step F)
=> status syntron_RFC step G & H are still to be reordered |
|
PRs of step A: (A001) simplify_workflow_Test - PR #411 - UPDATED moved to the end / rebase needed: The final state can be found at https://github.com/syntron/OMPython/tree/mergeA |
Yeah sounds good. |
OK - working on it ... |
|
@adeas31 would it make sence to have a unittest without windows for testing? (windows quite often fails on github) - see https://github.com/syntron/OMPython/tree/test_ubuntu_nightly for a definition I just to check the PRs listed below |
|
Updated list of open PRs based on this RFC:
=> all are single comit PRs; if needed, the detailed version can be found at https://github.com/syntron/OMPython/
|
|
A preliminary state including steps G is available:
and also a version including G & H - a possible baseline for v5.0.0: Feel free to test and/or provide feedback! |
No. We should fix the Windows github runner. |
OK; regarding this I'm out ... It fails at the step of downloading OMC / setup OMC |
This should work fine with #445. The actual fix is done here OpenModelica/setup-openmodelica#471 |
Overall, the direction looks great @syntron ! However, I’m running into a regression when trying to run a pre-compiled BouncingBall example. It seems the way the command line is constructed has changed in a way that the executable no longer accepts. The number in instantiatemodel refers to PR. def instantiatemodel404(modelName):
mod = ModelicaSystemRunner(work_directory='.')
mod.setup(model_name=modelName)
return mod
def instantiatemodel401(modelName):
mod = ModelicaSystemRunner(
modelname=modelName,
runpath='.',
)
return modThen, we instantiate the model: mod = instantiatemodelXXX(modelName='BouncingBall')
override_variables = {'e': 0.7,}
sim_options_override = {'stopTime': 3.0, 'stepSize': 0.03125}
resfilepathname = 'bounce_run001.mat'
mod.setParameters(**override_variables)
mod.setSimulationOptions(**sim_options_override)
mod.simulate(resultfile=resfilepathname, simflags=None,)This fails because it builds a bad command line: But it worked in PR #401 Could you please clarify if the usage of ModelicaSystemRunner has changed, or if this is an unintended side effect of the new command-line construction? I’m happy to do more testing once we get this "Hello World" case running! Thanks. |
|
@joewa the problem is the change of sim command line handling in OpenModelia (see PR #400 and PR #402). This is NOT included in PR #401. You should be able to solve this for older versions < 1.26 if you use I'm currently not able to test any of this; thus, please provide feedback! |
|
I missed that the interface has changed between omc 1.25 and 1.26. So I did: def instantiatemodel404(modelName):
oms = OMSessionRunner(version="1.25.5")
mod = ModelicaSystemRunner(session=oms, work_directory='.')
mod.setup(model_name=modelName)
return modThat fixed it. There is a key I think we should use it to switch automatically between the interfaces @syntron Will do more testing in the next days... |
Perfect, it works with omc 1.25.5: def instantiatemodel404(modelName):
mod = ModelicaSystemRunner(work_directory='.')
mod.setup(model_name=modelName)
return modIn the next step I'd like the simulation to read timeseries from a file. |
Please have a look in PR #447 - is it the code you are searching? (please keep in mind this is untested!) The code for 1.25 is not working anymore as there is some internal handling of the data to allow to use something like docker or WSL as backend. Thus, the CSV file has to be read and the data merge to the input data used for the model simulation. |
This is actually not what I wanted. I'm using I'm calling the simulation like this: mod.simulate(
resultfile=resfilepathname,
simargs={'csvInput': mod_csvFile}, # Old: simflags=f"-csvInput={mod_csvFile}",
# overrideaux='variableFilter="'+'|'.join(list(res_vars))+'"' # TODO: how to do this with ModelicaSystemRunner?
)but this was not working. The problem is that the simarg "csvInput" is overwritten in simulate_cmd (in modelica_system_abc.py) So I added a check if "csvInput" was already provided in the simargs by the user: # always define the result file to use
om_cmd.arg_set(key="r", val=result_file.as_posix())
if simargs:
om_cmd.args_set(args=simargs)
...
if self._inputs: # if model has input quantities
for key, val in self._inputs.items():
...
if "csvInput" not in simargs: # NEW: csvInput provided by user
# csvfile is based on name used for result file
csvfile = result_file.parent / f"{result_file.stem}.csv"
# write csv file and store the name
csvfile = self._createCSVData(csvfile=csvfile)
om_cmd.arg_set(key="csvInput", val=csvfile.as_posix())
return om_cmdDoes this make sense for you @syntron ? |
Makes sense ... But what against the following code: This is the current definition which allows to run code also on docker or WSL. There, the csv file could not be read from the local system but the data has to be handled internally by the ModelicaSystem* class such that is available to the model. Regarding the code you have, should the check not be on the level of However, this code needs checks for the case of docker or WSL ... these will find the file / path only if it is available for them! |
|
here are part G/H/I G => cleanup / all compatibility code moved to v4.0.0 part
H => final cleanup / for v5.0.0 (set as draft PR) I => additional changes; these could also be moved directly after part G
|
4 participants
Content (lots of changes / these will be splitted into several PRs if the general approach is approved):
Compared to PR #401 this solution:
|instead ofUnionFurther changes could do additional modifications such that the 3rd party packages (ZMQ) are not needed at all in the runner code path.
All tests are running without error / tested in https://github.com/syntron/OMPython/actions/runs/20865402893
Please comment!