Support yaml scenarios #275

markaren · 2019-06-18T18:23:42Z

This will be a controversial PR I guess. Uses YAML as the language for defining scenarios.
As you may see, the YAML parser API is almost identical to the JSON API. Furthermore it is backwards compatible.

closes #271

ljamt · 2019-06-19T06:54:13Z

I don't think it's a question about JSON or YAML, but more a question of prioritisation of time. Spending a few hours is of course no problem, but you still need to fix the cse-server-go to not break the demo application.

markaren · 2019-06-19T06:56:53Z

How is cse-server-go related to this (have not looked)? Replacing the current JSON scenarios can be done in seconds. But I'm not saying this should be merged prior to the next release, although it would be beneficial to users.

markaren · 2019-06-19T07:17:10Z

That last commit should probably be "fixed" upstream first. But at least this PR shows how easy it is to switch to YAML. What I really want is a discussion on if this is something we want or not. IMO JSON is not suited for this kind of use.

restenb · 2019-06-19T07:43:16Z

How is cse-server-go related to this (have not looked)? Replacing the current JSON scenarios can be done in seconds. But I'm not saying this should be merged prior to the next release, although it would be beneficial to users.

I guess the issue is mainly parsing. Both cse-server-go and cse-client currently assumes JSON as the format, so there's a good chunk of code to rehash if we now change it to YAML. The easiest approach is probably to parse the YAML back into JSON on the server, but that's not very fit for purpose either. That would assume such a parser already exists.

markaren · 2019-06-19T07:51:05Z

I have a hard time understanding why cse-client and cse-server would have to know anything about the scenario language except for the the file ending. But I will have a quick look myself.

restenb · 2019-06-19T07:58:41Z

I have a hard time understanding why cse-client and cse-server would have to know anything about the scenario language except for the the file ending. But I will have a quick look myself.

The view code for example goes straight off the incoming JSON data and turns that into a live table of ongoing scenario events. There are some implementation details there that would not be immediately compatible with a different format and therefore have to be rewritten.

It's not particularly difficult to do, it would just take time that could be spent elsewhere right now.

markaren · 2019-06-19T08:08:07Z

Since YAML is a superset of JSON, you can actually feed the original JSON into this parser and it will still work. So users can actually decide if they want to write JSON or YAML. This PR supports both.

But as @restenb points out, it boils down to spending time on the server/client. But we are in no hurry.

Edit 1:
My suggestion would be to keep using JSON in the front-end. In stead of passing the original file, we convert the parsed struct into JSON. The cavecat is that we would need both a JSON and YAML lib in the core.

Edit 2:
Or we expose the necessary information over the C API.

…' into feature/271-yaml-scenario-parser

…enario-parser

# Conflicts: # src/cpp/scenario_parser.cpp # test/data/scenarios/scenario1.json

# Conflicts: # conanfile.py

markaren · 2019-11-07T08:11:09Z

This PR has to receive a YAY or NAY tomorrow or it can just be closed.

kyllingstad

This looks good!

The only comment I have is that it should be tested with JSON as well as YAML input. If I remember last Friday's meeting correctly, we decided that we'd keep JSON support and continue to use JSON in the client for the time being. Hence, we should also test that it works.

markaren · 2019-11-11T07:51:43Z

That can be fixed!

markaren · 2019-11-11T08:36:41Z

Since I converted scenario_parser_test to a Boost.test module, this will break hard with PR #463

Edit: Well, hard and hard.. managable

markaren · 2019-11-11T08:48:56Z

@hplatou Can you confirm that it is a Jenkins/conan issue for the build failing on Windows w/o FMU-proxy and not something related to the changes in this PR?

kyllingstad

Looks good to me! Now all we need to do is figure out why the Jenkins build fails.

kyllingstad · 2019-11-11T08:53:50Z

@hplatou Can you confirm that it is a Jenkins/conan issue for the build failing on Windows w/o FMU-proxy and not something related to the changes in this PR?

Wild stab: Could it be because Findyaml-cpp.cmake is not included in the install() command in CMakeLists.txt?

markaren · 2019-11-11T08:55:30Z

What I think is strange is that the build output for the single failed build is full of fmuproxy references, while the build should not include fmuproxy.

markaren · 2019-11-11T08:59:30Z

But it is not clear to me if we should include Findyaml-cpp at all. It should not be necessary, but both me and Kristoffer has experienced cases where this file is not found by CMake (when we do not provide it).

kyllingstad · 2019-11-11T09:07:41Z

But it is not clear to me if we should include Findyaml-cpp at all. It should not be necessary, but both me and Kristroffer have experienced cases where this file is not found by CMake (when we do not provide it).

I tried installing the yaml-cpp Conan package on my computer now, and on both Windows and Linux it provides a CMake configuration file (yaml-cpp-config.cmake) in its package directory. Hence, the Findyaml-cpp.cmake script provided in this PR shouldn't really be necessary.

kyllingstad · 2019-11-11T09:25:35Z

I've also tried checking out this branch, deleting cmake/Findyaml-cpp.cmake and running a full configure&build. It works fine on Linux, but fails to find the package on Windows, as @markaren also experienced.

Looking at the generated conanbuildinfo.cmake, the reason seems to be that the yaml-cpp package directory for some reason is not added to the CONAN_CMAKE_MODULE_PATH variable, and therefore does not get added to CMAKE_PREFIX_PATH, which again means that find_package() is unable to locate yaml-cpp-config.cmake. I have no idea why, and unfortunately I don't have any more time to look into this right now, but this may be a lead for someone else.

src/cpp/scenario_parser.cpp

test/data/scenarios/scenario1.yml

cmake/Findyaml-cpp.cmake

Co-Authored-By: Kristoffer Eide <[email protected]>

markaren · 2019-11-11T10:35:04Z

After googling a little bit, it seems the current issues are related to multiple innstallations of yaml-cpp on the Jenskins server. yaml-cpp 0.6.2 from bincrafters and 0.6.3 from conan-center.

eidekrist · 2019-11-11T14:03:28Z

After googling a little bit, it seems the current issues are related to multiple innstallations of yaml-cpp on the Jenskins server. yaml-cpp 0.6.2 from bincrafters and 0.6.3 from conan-center.

I fixed it! 💪

markaren · 2019-11-11T14:10:37Z

After googling a little bit, it seems the current issues are related to multiple innstallations of yaml-cpp on the Jenskins server. yaml-cpp 0.6.2 from bincrafters and 0.6.3 from conan-center.

I fixed it! 💪

Nice!

markaren · 2019-11-12T08:10:51Z

🎂 🍾 🎈

Replace json with yaml for scenarios

56c5f6e

markaren requested review from kyllingstad, lassebje, ljamt, hplatou, mrindar and eidekrist June 18, 2019 18:23

Missing a comma somehow

3559b05

markaren requested a review from restenb June 19, 2019 06:48

This was referenced Jun 19, 2019

Provide overview of modified variables #212

Closed

#276 Now using name only to identify signals #277

Merged

markaren force-pushed the feature/271-yaml-scenario-parser branch from 69ac80d to 3559b05 Compare June 20, 2019 06:42

Halvor Platou and others added 6 commits June 21, 2019 09:52

Making Jenkins great again!

98aa8a0

Fix ubuntu dependecies

2cd1f60

Merge remote-tracking branch 'origin/feature/271-yaml-scenario-parser…

f064fd2

…' into feature/271-yaml-scenario-parser

Merge branch 'master' into feature/271-yaml-scenario-parser

d4f5bdb

Merge remote-tracking branch 'origin/master' into feature/271-yaml-sc…

d224ef1

…enario-parser

Merge branch 'master' into feature/271-yaml-scenario-parser

aa0d25b

# Conflicts: # src/cpp/scenario_parser.cpp # test/data/scenarios/scenario1.json

markaren changed the title ~~Replace json with yaml for scenarios~~ Support yaml scenarios Sep 5, 2019

markaren added 4 commits September 10, 2019 12:40

Merge branch 'master' into feature/271-yaml-scenario-parser

2b8dae7

# Conflicts: # conanfile.py

Replace nlohmann_json with yaml-cpp in find_dependency

cd02cc2

Update conanfile

007a76b

Fix copy of non-existent file

c59fc73

kyllingstad requested changes Nov 11, 2019

View reviewed changes

markaren added 2 commits November 11, 2019 09:15

Merge branch 'master' into feature/271-yaml-scenario-parser

27b2cec

Test both .json and .yml

facae6f

kyllingstad approved these changes Nov 11, 2019

View reviewed changes

Fix typo

14d91da

eidekrist reviewed Nov 11, 2019

View reviewed changes

src/cpp/scenario_parser.cpp Show resolved Hide resolved

eidekrist reviewed Nov 11, 2019

View reviewed changes

test/data/scenarios/scenario1.yml Outdated Show resolved Hide resolved

markaren added 2 commits November 11, 2019 10:57

Remove type and causality from .yml test file

8d2488f

yaml-cpp is now found in conan-center

e441145

eidekrist reviewed Nov 11, 2019

View reviewed changes

cmake/Findyaml-cpp.cmake Outdated Show resolved Hide resolved

Update cmake/Findyaml-cpp.cmake

504dbdb

Co-Authored-By: Kristoffer Eide <[email protected]>

ljamt approved these changes Nov 11, 2019

View reviewed changes

eidekrist approved these changes Nov 11, 2019

View reviewed changes

Update doc: yaml -> json or yaml

8f61fdf

markaren merged commit cfcddb3 into master Nov 12, 2019

markaren deleted the feature/271-yaml-scenario-parser branch November 12, 2019 16:06

kyllingstad mentioned this pull request Nov 25, 2024

Enable saving the entire simulation state to disk and restoring it again #757

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support yaml scenarios #275

Support yaml scenarios #275

markaren commented Jun 18, 2019 •

edited

Loading

ljamt commented Jun 19, 2019

markaren commented Jun 19, 2019

markaren commented Jun 19, 2019

restenb commented Jun 19, 2019 •

edited

Loading

markaren commented Jun 19, 2019 •

edited

Loading

restenb commented Jun 19, 2019

markaren commented Jun 19, 2019 •

edited

Loading

markaren commented Nov 7, 2019

kyllingstad left a comment

markaren commented Nov 11, 2019

markaren commented Nov 11, 2019 •

edited

Loading

markaren commented Nov 11, 2019

kyllingstad left a comment

kyllingstad commented Nov 11, 2019

markaren commented Nov 11, 2019

markaren commented Nov 11, 2019 •

edited

Loading

kyllingstad commented Nov 11, 2019

kyllingstad commented Nov 11, 2019

markaren commented Nov 11, 2019

eidekrist commented Nov 11, 2019

markaren commented Nov 11, 2019

markaren commented Nov 12, 2019

Support yaml scenarios #275

Support yaml scenarios #275

Conversation

markaren commented Jun 18, 2019 • edited Loading

ljamt commented Jun 19, 2019

markaren commented Jun 19, 2019

markaren commented Jun 19, 2019

restenb commented Jun 19, 2019 • edited Loading

markaren commented Jun 19, 2019 • edited Loading

restenb commented Jun 19, 2019

markaren commented Jun 19, 2019 • edited Loading

markaren commented Nov 7, 2019

kyllingstad left a comment

Choose a reason for hiding this comment

markaren commented Nov 11, 2019

markaren commented Nov 11, 2019 • edited Loading

markaren commented Nov 11, 2019

kyllingstad left a comment

Choose a reason for hiding this comment

kyllingstad commented Nov 11, 2019

markaren commented Nov 11, 2019

markaren commented Nov 11, 2019 • edited Loading

kyllingstad commented Nov 11, 2019

kyllingstad commented Nov 11, 2019

markaren commented Nov 11, 2019

eidekrist commented Nov 11, 2019

markaren commented Nov 11, 2019

markaren commented Nov 12, 2019

markaren commented Jun 18, 2019 •

edited

Loading

restenb commented Jun 19, 2019 •

edited

Loading

markaren commented Jun 19, 2019 •

edited

Loading

markaren commented Jun 19, 2019 •

edited

Loading

markaren commented Nov 11, 2019 •

edited

Loading

markaren commented Nov 11, 2019 •

edited

Loading