Skip to content

Rewrite OMCSessionZMQ #295

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

Merged
merged 10 commits into from
Jun 18, 2025
Merged

Conversation

syntron
Copy link
Contributor

@syntron syntron commented May 28, 2025

This is now a complete rewrite of OMCSessionZMQ; it is defined on top of PR #300

The main items are still the same as mentioned in the original statement.

Besides the OMCSessionZMQ class there are now several classes which can be used to create the OMC/ZMQ process:

  • OMCProcessPort
  • OMCProcessLocal
  • OMCProcessDocker
  • OMCProcessDockerContainer

This will also allow usage of WSL (see #268); I have some code ready but could not test it ...


OMCSessionZMQ handles different tasks:

a) start OMC process (local / via docker / via docker container)
b) connect to OMC process via ZMQ
c) connect to OMC using ZMQ

This PR tries to find a way to to split these items into several classes:

[1] OMCSessionZMQ - for b) and c)
[2] OMCProcess / OMCProcessLocal / OMCProcessDummy / OMCProcessDocker(?) - for a)

I started to work on this due to the docker code just sitting there without much usage in my case. Save for docker, all other test run fine ...

Still work in progress; feel free to comment if this even makes sense ...

On top of PR #291

@syntron syntron force-pushed the OMCSessionZMQDocker branch 3 times, most recently from e41076d to 1af654a Compare June 10, 2025 15:47
@syntron syntron mentioned this pull request Jun 10, 2025
@syntron syntron force-pushed the OMCSessionZMQDocker branch from 1af654a to ee230a3 Compare June 10, 2025 16:10
@adeas31 adeas31 added this to the 3.8.0 milestone Jun 11, 2025
@syntron
Copy link
Contributor Author

syntron commented Jun 11, 2025

@adeas31 I just tried to run OMPython with docker ... and it failed even for 3.6.0! At that time docker was last tested? Who is using it?

PS: got it working again ... it depends on omc installed on the host (in 3.6.0 and also now)

@syntron syntron force-pushed the OMCSessionZMQDocker branch 2 times, most recently from 6250241 to 3bf8811 Compare June 11, 2025 21:09
@syntron
Copy link
Contributor Author

syntron commented Jun 11, 2025

@adeas31 this is now a complete rewrite of the code to connect to / start OMC in different ways:

(1) just provide the ZMQ port => OMCProcessPort
(2) local OMC => OMCProcessLocal
(3) in docker => OMCProcessDocker
(4) via docker container ID => OMCProcessDockerContainer

I think, what it would be quite easy to add podman or similar tools based on this definition. For the default users (localy started OMC), nothing changes.

Question: is this something for the next release? If yes, I would cleanup this code and prepare it as PR ... however, more testing would be needed! I can only test a limited set of possibilities ... port (win/linux) / local (this is tested on github) / docker (linux) / docker container (linux)

@syntron syntron force-pushed the OMCSessionZMQDocker branch from 3bf8811 to 99572e4 Compare June 12, 2025 19:28
@syntron syntron changed the title Cleanup OMCSessionZMQ Rewrite OMCSessionZMQ Jun 12, 2025
@syntron syntron marked this pull request as ready for review June 12, 2025 19:36
@syntron
Copy link
Contributor Author

syntron commented Jun 12, 2025

@adeas31 Please test and consider for the next release - it is quite a big change! (see #254)

This was referenced Jun 12, 2025
@syntron syntron force-pushed the OMCSessionZMQDocker branch from 99572e4 to ae4855c Compare June 12, 2025 19:51
@adeas31 adeas31 requested review from arun3688 and adeas31 June 16, 2025 12:23
Copy link
Member

@adeas31 adeas31 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

On Windows, use the list input style of the subprocess module to
avoid problems resulting from spaces in the path string.
Linux, however, only works with the string version.
super().__init__(timeout=timeout)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps you can define a proper constructor for OMCProcessDockerHelper which sets the docker arguments using the super().__init__ instead of setting them explicitly in OMCProcessDocker and OMCProcessDockerContainer.

super().__init__ can handle it based on cooperative multiple inheritance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some updates for this series including the requested change; however, I'm not sure if it would be good to put all into one PR. I prepared it as PR #306 (includes #301)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. Please rebase the PR and we are good to merge it.

@adeas31 adeas31 modified the milestones: 3.8.0, 4.0.0 Jun 17, 2025
@syntron syntron force-pushed the OMCSessionZMQDocker branch from ae4855c to 1fe63c8 Compare June 17, 2025 18:47
@adeas31 adeas31 merged commit a5c8a83 into OpenModelica:master Jun 18, 2025
5 checks passed
@syntron syntron deleted the OMCSessionZMQDocker branch June 19, 2025 19:45
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