Skip to content
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

Re-use script from OSI repo #55

Closed
jdsika opened this issue Jan 11, 2024 · 9 comments
Closed

Re-use script from OSI repo #55

jdsika opened this issue Jan 11, 2024 · 9 comments
Assignees
Labels
quality Quality improvements. question General questions about the standard, work-flow and code.

Comments

@jdsika
Copy link
Contributor

jdsika commented Jan 11, 2024

I am asking myself, if we should have a script in OSI handling traces and in this repo again? Souldn't we have ONE place that shows how to handle traces and then re-use it?
https://github.com/OpenSimulationInterface/open-simulation-interface/blob/master/format/OSITrace.py

@jdsika jdsika added the question General questions about the standard, work-flow and code. label Jan 11, 2024
@jdsika jdsika added the quality Quality improvements. label Jan 15, 2024
@jdsika
Copy link
Contributor Author

jdsika commented Jan 15, 2024

  • Compare read/write methods in OSI and osi-validation
  • Improve OSITrace.py class in OSI repo (and examples demonstrating it in osi repo)
  • Enable lzma support
  • Add test files (different proto versions): 3.0.0, 3.15.x, latest
  • Create PR in OSI
  • Remove write scripts reading/writing in osi-validation
  • Use class from osi repo

see OpenSimulationInterface/open-simulation-interface#758

@jdsika
Copy link
Contributor Author

jdsika commented Jan 15, 2024

It would be nice if the different osi trace formats described iun the documentation do also have script examples along with them.

@jdsika
Copy link
Contributor Author

jdsika commented Jan 18, 2024

@ClemensLinnhoff Protobuffer changed its version schema (OMG). The question: why?
https://github.com/protocolbuffers/protobuf/releases/tag/v25.2

What does that mean for the "on the wire compatibility"? What breaks?

@jdsika
Copy link
Contributor Author

jdsika commented Jan 18, 2024

@ClemensLinnhoff
Copy link
Contributor

@ClemensLinnhoff Protobuffer changed its version schema (OMG). The question: why? https://github.com/protocolbuffers/protobuf/releases/tag/v25.2

What does that mean for the "on the wire compatibility"? What breaks?

They changed it in v21. It says in the release notes:

  • Versioning scheme is changing to decouple major versions for languages
  • Minor and patch versions will remain coupled

That means, that it is 3.21.0 for C++ but 4.21.0 for python.

Why you ask? Probably because they don't like the people who have to work with protobuf 😄

@ClemensLinnhoff
Copy link
Contributor

CMAKE says protobuf 2.6.1: https://github.com/OpenSimulationInterface/open-simulation-interface/blob/master/CMakeLists.txt

Well 2.6.1 is now almost 10 years old. Should it be updated in the CMakeLists? Or do we want to support 10 year old trace files?

@ClemensLinnhoff
Copy link
Contributor

ClemensLinnhoff commented Jan 18, 2024

@jdsika I have generated some test trace files, e.g. with different protobuf versions. Where would you put them? Should we put them in the open-simulation-interface repo to perform unit tests on OSITrace and the formatting tools? Or should we put them in the osi-validation repo to test the validator?

Of course we could also put them in the open-simulation-interface and use them also for the validator.
And if we put them there, would you put them in the tests folder or in the format folder? Or in a completely separate trace_file_examples folder?

@jdsika
Copy link
Contributor Author

jdsika commented Jan 19, 2024

CMAKE says protobuf 2.6.1: https://github.com/OpenSimulationInterface/open-simulation-interface/blob/master/CMakeLists.txt

Well 2.6.1 is now almost 10 years old. Should it be updated in the CMakeLists? Or do we want to support 10 year old trace files?

We should keep the OSI Standard working with Proto 2.6.1. This does not mean that we do not recommend a newer proto version e.g. through using it in the pipeline OR that we PIN and recommend a specific version e.g. in a project using OSI like osi-validation. So you see these are three different topics somehow.

I will come back to you regarding where to put the traces but my initial thought would be that they are part of OSI and that there is a set of unit tests that e.g. use OSITrace including those traces. Also those traces could then be used for test purposes of other projects having osi as a git submodule. I think we should put some thought into what should be the content of those traces. For osi-validation we somehow need to test the range of signals that have constraints through rules AND the overall set of rules. What do you think?

@ClemensLinnhoff
Copy link
Contributor

To test OSItrace we need different protobuf versions and different trace file formats (.osi, .txt and .lzma). I think the actual contents of the trace file is not really relevant for that.
Then we can also use these files to test if osi-validation can handle the protobuf version and the different formats.

For osi-validation, we then need separate trace files for testing the behavior of the validator. However, we could generate these trace files on the fly, as e.g. done in the osi_trace unittest: https://github.com/OpenSimulationInterface/open-simulation-interface/blob/master/tests/test_osi_trace.py.
However, we also need a possibility to test the outcome of osi-validation, which the current unit tests don't.
Since we need to think about what should actually be tested and it might change in the near future (see discussions about required fields), I would suggest to implement this kind of testing in a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
quality Quality improvements. question General questions about the standard, work-flow and code.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants