Skip to content

Conversation

@AxelMatstoms
Copy link
Contributor

If the session kwarg is specified, the ModelicaSystem will use that session instead of creating a new one. This is useful for creating a ModelicaSystem for a model that is already loaded in an already existing session.

Related Issues

Purpose

The purpose of the change is to allow usage of the high level ModelicaSystem API around models that are already loaded, or created through createModel etc in an existing session.

Approach

By adding a session keyword argument to the ModelicaSystem constructor, users can use the ModelicaSystem constructor as follows to construct a ModelicaSystem around an already loaded model from an existing session:

omc = OMCSessionZMQ()
omc.sendExpression("createModel(ExampleModel)")
...
model = ModelicaSystem(None, "ExampleModel", session=omc)
model.simulate()

@CLAassistant
Copy link

CLAassistant commented Mar 6, 2025

CLA assistant check
All committers have signed the CLA.

@adeas31
Copy link
Member

adeas31 commented Mar 6, 2025

We should do something about the existing session then. It is not good to keep using the same session from two different instances. Maybe kind of transfer the ownership to ModelicaSystem so the session will be used and destroyed only by ModelicaSystem instance and will not be used by the old instance.

@AxelMatstoms
Copy link
Contributor Author

@adeas31 Are there any other concerns with the ownership other than the __del__ methods that would prevent sharing ownership of a single session? Otherwise, an easy solution would be to remove the __del__ method from ModelicaSystem. The __del__ method on the session will be called when the ModelicaSystem goes out of scope anyways.

If not I could update the PR removing the del method. I also realized I forgot to set the session in the case the init method was called with zero positional arguments.

If the session kwarg is specified, the ModelicaSystem will use that
session instead of creating a new one. This is useful for creating a
ModelicaSystem for a model that is already loaded in an already existing
session.

Remove __del__ method from ModelicaSystem. Having the deleter there
meant that the deleter for the session would run multiple times if a
session had shared ownership. The deleter will still run when the
session's reference count reaches zero, or when it is garbage collected.
@AxelMatstoms
Copy link
Contributor Author

Updated to remove the deleter from ModelicaSystem. I've done some testing for this. Previously the session deleter would run multiple times for the same process pid. Removing the deleter from ModelicaSystem ensures it is run at most once (since Python doesn't guarantee that the del method will be called on objects that still exist at the end of the program1).

Footnotes

  1. https://docs.python.org/3/reference/datamodel.html#object.__del__

@adeas31 adeas31 requested a review from arun3688 March 7, 2025 11:30
@adeas31
Copy link
Member

adeas31 commented Mar 7, 2025

Maybe we should not remove the __del__ method, it is fine to call it once ModelicaSystem goes out of scope.

I tested the following and it works fine,

from OMPython import OMCSessionZMQ
omc = OMCSessionZMQ()
omc.sendExpression("createModel(ExampleModel)")
from OMPython import ModelicaSystem
model = ModelicaSystem(None, "ExampleModel", session=omc)
model.buildModel()

However, the following doesn't work,

from OMPython import OMCSessionZMQ
omc = OMCSessionZMQ()
omc.sendExpression("createModel(ExampleModel)")
from OMPython import ModelicaSystem
model = ModelicaSystem(None, "ExampleModel", session=omc)
omc.sendExpression("quit()")
model.buildModel()

This gives the exception error,

OMPython - ERROR - Exception <class 'Exception'> raised: Process Exited, No connection with OMC. Create a new instance of OMCSession
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\OpenModelica\OMPython\OMPython\__init__.py", line 1038, in buildModel
    self._check_error()
  File "C:\OpenModelica\OMPython\OMPython\__init__.py", line 1013, in _check_error
    errstr = self.getconn.sendExpression("getErrorString()")
  File "C:\OpenModelica\OMPython\OMPython\__init__.py", line 813, in sendExpression
    raise Exception("Process Exited, No connection with OMC. Create a new instance of OMCSession")
Exception: Process Exited, No connection with OMC. Create a new instance of OMCSession

I guess it is expected output as the omc session that ModelicaSystem is using has been closed. The only way forward from here is to create a new omc session and new ModelicaSystem.

@arun3688 what do you think?

@AxelMatstoms
Copy link
Contributor Author

I think the problem with having the del method on ModelicaSystem is that it gets called twice: once when the ModelicaSystem goes out of scope, and once when the session goes out of scope.

I tried the following on the upstream master branch:

from OMPython import ModelicaSystem, OMCSessionBase

deleter = OMCSessionBase.__del__
def new_deleter(self):
    print("__del__:", self._omc_process.pid)
    deleter(self)
OMCSessionBase.__del__ = new_deleter

model = ModelicaSystem(None, "Modelica.Blocks.Examples.PID_Controller")

which gives this output showing that the deleter runs twice. Removing the deleter from ModelicaSystem fixes this.

__del__: 105196
__del__: 105196

@adeas31 adeas31 enabled auto-merge (squash) March 20, 2025 13:11
@adeas31 adeas31 merged commit 3ba72b0 into OpenModelica:master Mar 20, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants