Skip to content

Finish error handling - #278

Merged
adeas31 merged 7 commits into
OpenModelica:masterfrom
syntron:finish_error_handling
May 22, 2025
Merged

Finish error handling#278
adeas31 merged 7 commits into
OpenModelica:masterfrom
syntron:finish_error_handling

Conversation

@syntron

Copy link
Copy Markdown
Contributor

Finish update of error handling in OMSession.py and ModelicaSystem.py

Based on PR #277, #273 and #270

Changes

  • Always check for errors in sendExpression()
  • align logging - log data only once
  • cleanup

This was referenced May 1, 2025
Closed
@syntron
syntronforce-pushed the finish_error_handling branch from 5377e62 to 47cd01bCompareMay 3, 2025 19:34
@syntronsyntron mentioned this pull request May 6, 2025
@adeas31

Copy link
Copy Markdown
Member

Rebase this one and see if you can use getMessagesStringInternal(). We should also check if expression is getErrorString() or getMessagesStringInternal() then we should not call it twice.

@syntron

Copy link
Copy Markdown
ContributorAuthor

@adeas31 would it be possible to merge #281 and #282 before that? Both would merge in this order without problems even if not rebased.

This would simplify the merge as this PR is based on these two. Furthermore, I will be limited in time till mid of next week ...

@syntron

Copy link
Copy Markdown
ContributorAuthor

@adeas31 do you have a simple way / an example to force an error? I would like to check how to evaluate the output of getMessagesStringInternal()

@adeas31

Copy link
Copy Markdown
Member

Run the following script with omc,

loadString("model M Real foo = \"str\"; annotation (uses(Modelica(version=\"4.0.0\"))); end M;");
getMessagesStringInternal();
checkModel(M);
getMessagesStringInternal();

equivalent OMPython code is,

fromOMPythonimportOMCSessionZMQomc=OMCSessionZMQ()
omc.sendExpression("model M Real foo = \"str\"; annotation (uses(Modelica(version=\"4.0.0\"))); end M;")
omc.sendExpression("getMessagesStringInternal()")
omc.sendExpression("checkModel(M)")
omc.sendExpression("getMessagesStringInternal()")

The first call to getMessagesStringInternal() gives the notificaitons caused by loading model M, the second call gives the error caused by calling checkModel(M).
Note. I haven't tried running the python code but I guess it will work.

You can also see how getMessagesStringInternal() is used in OMEdit here https://github.com/OpenModelica/OpenModelica/blob/master/OMEdit/OMEditLIB/OMC/OMCProxy.cpp#L611

@syntron
syntronforce-pushed the finish_error_handling branch 2 times, most recently from 8f6d05d to 31ef902CompareMay 8, 2025 20:26
@adeas31

Copy link
Copy Markdown
Member

Let me know once its ready for review.

@syntron
syntronforce-pushed the finish_error_handling branch from 31ef902 to fe5f3e3CompareMay 9, 2025 19:03
@syntron

syntron commented May 9, 2025

Copy link
Copy Markdown
ContributorAuthor

Let me know once its ready for review.

@adeas31 all done including getMessagesStringInternal() in this PR

Please use the following order for review: PR #281, PR #282 (needs a simple conflict solution), PR #278

PR #279 is currently not updated as it would be on top of PR #278

Besides these, there are some smaller cleanups pending

@syntron
syntronforce-pushed the finish_error_handling branch from 08d1007 to 5d9b182CompareMay 12, 2025 16:16
@adeas31

Copy link
Copy Markdown
Member

All the dependent PRs are merged. Lets rebase this one now.

@syntron
syntronforce-pushed the finish_error_handling branch from 5d9b182 to 4d8a0e7CompareMay 20, 2025 19:23
@syntron

Copy link
Copy Markdown
ContributorAuthor

@adeas31 ready to go ...

@adeas31adeas31 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

self._connect_to_omc(timeout)

self._re_log_entries = None
self._re_log_raw = None

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we really need to make them class members?

Comment threadOMPython/OMCSession.py Outdated
error_raw = self._omc.recv_string()
# run error handling only if there is something to check
if error_raw != "{}\n":
if not self._re_log_entries:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

self._re_log_entries and self._re_log_raw can be local variables here.

@syntronsyntronMay 21, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I defined them as class variables to do the initialisation once (if needed at all). Later, the compiled re struture is just reused ... However, if the expected numbers of calls to this part is low, it makes sense to run this each time.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The actual re compilation is run each time, its just they are initialized to none only once. I don't think initializing them to none locally is expensive.

@syntronsyntronMay 21, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

in OMCSession.py line 584ff (definition of sendExpression()):

if not self._re_log_entries:
self._re_log_entries = re.compile(...)
if not self._re_log_raw:
self._re_log_raw = re.compile(...)

Compilation is done only once (on first call; self._re* class variables are None); after that, the variables are set (not None) and, thus, the compiled version is used ...

@syntron
syntronforce-pushed the finish_error_handling branch from 4d8a0e7 to 94ec48bCompareMay 21, 2025 16:50
@adeas31
adeas31 merged commit 64a16ef into OpenModelica:masterMay 22, 2025
@syntron
syntron deleted the finish_error_handling branch May 22, 2025 14:52
Sign up for freeto 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.

2 participants

@syntron@adeas31