Skip to content

OMParser update#469

Open
syntron wants to merge 15 commits into
OpenModelica:masterfrom
syntron:OMParser_update
Open

OMParser update#469
syntron wants to merge 15 commits into
OpenModelica:masterfrom
syntron:OMParser_update

Conversation

@syntron

@syntron syntron commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

update OMParser; mainly just change code updates to get it safe for reuse in ModelicaDoE usecases

  • please check as I do not have all details of the code workflow
  • should be after PR F001 restructure cleanup #444 but it is save anytime as there is no other PR touching OMParser.py

@syntron

syntron commented Jun 28, 2026

Copy link
Copy Markdown
Contributor Author

@adeas31 The current code still uses Python2 definitions - thus it fails:

    def formatRecords(strings):
        result['RecordResults'] = {}
        recordName = strings[strings.find("record ") + 1:strings.find("\n")]
        recordName = recordName.replace("ecord ", '').strip()
        strings = strings.replace(("end " + recordName + ";"), '').strip()
        recordItems = strings[strings.find("\n") + 1: len(strings)]
>       recordItems = recordItems.translate(None, "\\")
E       TypeError: str.translate() takes exactly one argument (2 given)

Should it be converted to Python3 at all or removed? This PR tries to update the code to get it running (see: 1b592c6 for Python2 => Python3 conversion) and thread save but is it still be required or should it be completly removed? Overall it was not working for some time / any Python3 usage

@adeas31

adeas31 commented Jun 29, 2026

Copy link
Copy Markdown
Member

As far as I can see the code is used to format the records, the record returned by simulate API is handled via formatSimRes. The other APIs that return a record are getMessagesStringInternal and checkSettings or maybe more. You can test getMessagesStringInternal and see if its result record is formatted correctly.

I recommend converting the code to python 3 and adding a test for it.

@syntron syntron force-pushed the OMParser_update branch 2 times, most recently from 28e5a18 to 6c74ec7 Compare June 29, 2026 19:03
@syntron

syntron commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

@adeas31 I'm not that familar with the OM return values to check this in detail; regarding OMParser, I noted that it it does not return the expected results:

running test_everything from test_OMParser (currently commented out; content of results)

{'RecordResults': {',5.9,6,NONE ( )},record AB': '"', 'startTime': 'ErrorLevel.warning', "'stop*Time'": 'SOME(1.0', 'nd ABC;}': 'end ABC;}', 'RecordName': ''}}

the same test in test_OMTypedParser:

(1.0, ((1, True, 3), ('4"\n', 5.9, 6, None), {'startTime': 'ErrorLevel.warning', "'stop*Time'": 1.0}))

It is the same input data to parse! The changes in this PR have no effect on results of test_OMParser.test_everything() - it is the same results data before / after applying it; just 675997b is always needed to fix Python 2 code.

Input data as reference:

    testdata = """
    (1.0,{{1,true,3},{"4\\"
",5.9,6,NONE ( )},record ABC
    startTime = ErrorLevel.warning,
    'stop*Time' = SOME(1.0)
end ABC;})
    """

Comment thread README.md

- Python 3.x supported
- PyZMQ is required
- Python >= 3.10 supported with complete functionality for Python >= 3.12

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.

Can't we simply say Python >= 3.10 or Python >= 3.12 supported. Do we need to be verbose here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we can go with Python >= 3.10; however, the OMPath functionality is only supported for Python >= 3.12 - Python 3.10 has only a compatibility layer to get something working

PS: this should be in the discussion for PR #444

@adeas31

adeas31 commented Jun 30, 2026

Copy link
Copy Markdown
Member

Can this be merged now or does it depend on any other PR?

@syntron

syntron commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

@adeas31 it can be merged as I hope to not change any functionality; however, I did only some basic tests as I have no real use cases for this part of the code ...

PR #444 should be run before - it is just some simple / small cleanup ...; else it is included in this PR ...

@syntron

syntron commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

I just run some checks; the return values from OMParser would crash OMPython ...

Example from run of test_ModelicaSystemOMC.test_getSolutions():

print(expr) => buildModel(M, variableFilter=".*")
print(result) => {"/tmp/88ab0555-6bbe-4470-b43e-2d59df7e50d4/M", "M_init.xml"}
print(om_parser_typed) => ('/tmp/88ab0555-6bbe-4470-b43e-2d59df7e50d4/M', 'M_init.xml')
print(om_parser_basic) => {'SET1': {'Values': ['"/tmp/88ab0555-6bbe-4470-b43e-2d59df7e50d4/M", "M_init.xml"']}}

The data expected by the code is given by om_parser_typed; om_parser_basic would crash the code if returned; nevertheless, it contains the same / the expected information ...

Did I missed some additional conversion, i.e. SET1 => set()?

(test run with both versions of the code - same results!)

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.

2 participants