Fix #4885 - Rewrite MeasureManager in C++ #4920

jmarrec · 2023-06-23T12:31:09Z

Pull request overview

Fix Rewrite MeasureManager in C++ #4885

Note: this includes Add a swig out typemap for Json::Value + openstudio::path typemap in/out for Python + add toJSON() for Workfow files and BCLMeasure #4917

Pull Request Author

Model API Changes / Additions
Any new or modified fields have been implemented in the EnergyPlus ForwardTranslator (and ReverseTranslator as appropriate)
Model API methods are tested (in src/model/test)
EnergyPlus ForwardTranslator Tests (in src/energyplus/Test)
If a new object or method, added a test in NREL/OpenStudio-resources: Add Link
If needed, added VersionTranslation rules for the objects (src/osversion/VersionTranslator.cpp)
Verified that C# bindings built fine on Windows, partial classes used as needed, etc.
All new and existing tests passes
If methods have been deprecated, update rest of code to use the new methods

Labels:

If change to an IDD file, add the label IDDChange
If breaking existing API, add the label APIChange
If deemed ready, add label Pull Request - Ready for CI so that CI builds your PR

Review Checklist

This will not be exhaustively relevant to every PR.

Perform a Code Review on GitHub
Code Style, strip trailing whitespace, etc.
All related changes have been implemented: model changes, model tests, FT changes, FT tests, VersionTranslation, OS App
Labeling is ok
If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
If feature, test running new feature, try creative ways to break it
CI status: all green or justified

….0.0: on unix at least it works wiht localhost, 0.0.0.0 and 127.0.0.1

when "stack too deep" is the err string, the location `rb_eval_string("[email protected]_s");` will fail

The child class Ctor will intialize it

…p error

…e MeasureManager too

… fail when calling arguments(model) for an OLD reporting measure

kbenne · 2023-08-10T00:35:24Z

@jmarrec. Check it out 3c4a1c3 I think this does the trick. I thnk now we just need to do a bunch of testing to make sure the API and behavior is consistent with the original Ruby implementation.

@DavidGoldwasser

…hread does the actual ruby/python work

… missing)

… files

…r_c++

src/cli/MeasureManager.cpp

kbenne · 2023-08-14T19:24:25Z

src/cli/MeasureManager.cpp

@@ -634,63 +614,54 @@ void MeasureManagerServer::handle_post(web::http::http_request message) {
  const std::string uri = toString(web::http::uri::decode(message.relative_uri().path()));


Can message be passed by reference?

Should it be passed by reference is the question. All of the example code passes it by value.
Digging down the source code, this is a PIMPL class. Single member, Source: https://github.com/microsoft/cpprestsdk/blob/9c654889efb6f5bda69fe8f52b3f7d3d3fb56cd8/Release/include/cpprest/http_msg.h#L1573

std::shared_ptr<http::details::_http_request> _m_impl;

So that'd be the size of two pointers (data + control block) right?

https://github.com/microsoft/cpprestsdk/blob/9c654889efb6f5bda69fe8f52b3f7d3d3fb56cd8/Release/samples/CasaLens/casalens.cpp#L85

Something trivially_copyable should be passed by val if it's < 16 bytes. The reference count needs to be +1 then -1 here since it's not a POD type, but I would reckon the effect is minimal.

TL;DR: I don't think it matters. But if you prefer passing by ref / const ref, we can do that.

Case closed.

I guess it is slightly faster though. Here's a benchmark: https://quick-bench.com/q/gek5jMY8CDeiaU41SZIU30beUjM

Same locally on my ubuntu gcc

jmarrec · 2023-08-24T13:04:22Z

@kbenne mac CI has the same crash as I do locally when trying to run a Python measure first THEN a Ruby measure: https://ci.openstudio.net/blue/organizations/jenkins/openstudio-incremental-osx/detail/PR-4920/21/pipeline#step-76-log-7

kbenne · 2023-08-25T16:50:47Z

@kbenne mac CI has the same crash as I do locally when trying to run a Python measure first THEN a Ruby measure: https://ci.openstudio.net/blue/organizations/jenkins/openstudio-incremental-osx/detail/PR-4920/21/pipeline#step-76-log-7

I opened a new issue about this, because I think it is a separate issue from the Measure Manager itself. #4945

jmarrec · 2023-08-25T16:59:47Z

Definitely not caused by this PR.
Should we merge this one then?

kbenne · 2023-08-25T17:47:31Z

@kflemin is testing right now. After she is done, then I think we can merge @jmarrec

…into 4885_MeasureManager_c++

src/cli/MeasureManager.cpp

Use `std::fflush(std::cout)` instead of `std::cout << std::flush` Co-authored-by: Julien Marrec <[email protected]>

This issue only pertained to Mac and was occuring when a Python Measure was run before a Ruby Measure. The fix is to set the dlopen flags to RTLD_LOCAL prior to importing OpenStudio in Python. Test OpenStudioCLI.Labs.Run_PythonRuby demonstrates the original issue and will now pass. close #4945

…nStudio into 4885_MeasureManager_c++

ci-commercialbuildings · 2023-09-27T08:02:49Z

CI Results for 22d99c4:

Ubuntu 20.04:
Ubuntu 22.04:
- OpenStudio-3.7.0-alpha+22d99c4596-Ubuntu-22.04-x86_64.deb
- OpenStudio-3.7.0-alpha+22d99c4596-Ubuntu-22.04-x86_64.tar.gz
Darwin:
- OpenStudio-3.7.0-alpha+22d99c4596-Darwin-x86_64.dmg
- OpenStudio-3.7.0-alpha+22d99c4596-Darwin-x86_64.tar.gz
Windows:
- OpenStudio-3.7.0-alpha+22d99c4596-Windows.exe
- OpenStudio-3.7.0-alpha+22d99c4596-Windows.tar.gz
Code Coverage:

jmarrec added component - CLI Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. component - Ruby bindings component - Python bindings labels Jun 23, 2023

jmarrec requested review from kbenne, DavidGoldwasser and tijcolem June 23, 2023 12:31

jmarrec self-assigned this Jun 23, 2023

jmarrec force-pushed the 4885_MeasureManager_c++ branch 5 times, most recently from 74389ce to ae9c047 Compare June 30, 2023 07:04

jmarrec added 17 commits July 4, 2023 15:40

WIP

742f682

WIP2

5f54972

Implement getIdf

fe0bf72

WIP implementing getAndUpdateMeasure inside MeasureManager with caching

7a0ef58

More WIP putting all the pieces together

6d4d3c2

passing http://localhost was binding to tcp6, not tcp4, change to 0.0…

f505532

….0.0: on unix at least it works wiht localhost, 0.0.0.0 and 127.0.0.1

Use pytest to test the measure manager. Starting to look good!

7ddca5d

Add OSArgument::toJSON

e436183

Add measure_dir to BCLMeasure/BCLXML toJSON

93b0b34

Almost working! Ruby compute argument crashes...

dc9d8bd

Avoid crashing when trying to get location when ruby engine eval failed

8bd9138

when "stack too deep" is the err string, the location `rb_eval_string("[email protected]_s");` will fail

Add MeasureType OSMeasure::measureType() const

bf70f1e

The child class Ctor will intialize it

change the way to infer the ruby className because of a stack too dee…

6837b05

…p error

Try to move inferClassName / loadMeasure at ScriptEngine level

2d8bb96

Dry it up: Use the new methods, and in the measuresubcommand use th…

be003fa

…e MeasureManager too

Use conftest pytest from 9743a6b

f59cf9f

Wrap the code that queries the measure in a try-catch after having it…

2dbba58

… fail when calling arguments(model) for an OLD reporting measure

jmarrec added 8 commits August 14, 2023 16:13

Widden paths on windows when passing them to python.

7c37fa0

Remove interrupt handler, not used anymore.

29d7726

Use a member function pointer instead of a std::function

eb1c3ca

Add comments explaining the shenanigans done to ensure the **main** t…

eaeed53

…hread does the actual ruby/python work

Handle incorrect "set" my_measures_dir gracefully (wrong directory or…

449c70c

… missing)

Missing early return.

3d1bf85

Move ThreadSafeDeque into its own header to avoid polluting all other…

b84a532

… files

Merge remote-tracking branch 'origin/develop' into 4885_MeasureManage…

3e228f9

…r_c++

kbenne approved these changes Aug 14, 2023

View reviewed changes

Remove debug messages

9b31f65

jmarrec added a commit to openstudiocoalition/OpenStudioApplication that referenced this pull request Aug 16, 2023

Track the MeasureManager PR NREL/OpenStudio#4920

3f54597

jmarrec added 3 commits August 18, 2023 18:03

/compute_arguments endpoint must take ONLY an OSM

6343a48

Forgot a debug message

2a7c1cf

Add a ctest for running a Python measure FIRST then a ruby: crash on mac

b58c33d

kbenne added 2 commits September 22, 2023 17:22

Add (and flush) a stdout message on MeasureManager ready

13cbc82

Merge branch '4885_MeasureManager_c++' of github.com:NREL/OpenStudio …

e1f5e0c

…into 4885_MeasureManager_c++

jmarrec commented Sep 25, 2023

View reviewed changes

src/cli/MeasureManager.cpp Outdated Show resolved Hide resolved

kbenne and others added 5 commits September 25, 2023 13:30

Update src/cli/MeasureManager.cpp

dd55a86

Use `std::fflush(std::cout)` instead of `std::cout << std::flush` Co-authored-by: Julien Marrec <[email protected]>

Merge branch '4885_MeasureManager_c++' of https://github.com/NREL/Ope…

109be9d

…nStudio into 4885_MeasureManager_c++

Fix fflush

f4d43bf

Merge branch 'develop' into 4885_MeasureManager_c++

22d99c4

jmarrec merged commit b7fb557 into develop Sep 28, 2023
2 checks passed

jmarrec deleted the 4885_MeasureManager_c++ branch September 28, 2023 06:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4885 - Rewrite MeasureManager in C++ #4920

Fix #4885 - Rewrite MeasureManager in C++ #4920

jmarrec commented Jun 23, 2023 •

edited

Loading

kbenne commented Aug 10, 2023

kbenne Aug 14, 2023

jmarrec Aug 14, 2023 •

edited

Loading

jmarrec Aug 14, 2023

jmarrec Aug 14, 2023 •

edited

Loading

kbenne Aug 14, 2023

jmarrec Aug 14, 2023

jmarrec commented Aug 24, 2023

kbenne commented Aug 25, 2023

jmarrec commented Aug 25, 2023 •

edited

Loading

kbenne commented Aug 25, 2023

ci-commercialbuildings commented Sep 27, 2023 •

edited

Loading

		@@ -634,63 +614,54 @@ void MeasureManagerServer::handle_post(web::http::http_request message) {
		const std::string uri = toString(web::http::uri::decode(message.relative_uri().path()));

Fix #4885 - Rewrite MeasureManager in C++ #4920

Fix #4885 - Rewrite MeasureManager in C++ #4920

Conversation

jmarrec commented Jun 23, 2023 • edited Loading

Pull request overview

Pull Request Author

Review Checklist

kbenne commented Aug 10, 2023

kbenne Aug 14, 2023

Choose a reason for hiding this comment

jmarrec Aug 14, 2023 • edited Loading

Choose a reason for hiding this comment

jmarrec Aug 14, 2023

Choose a reason for hiding this comment

jmarrec Aug 14, 2023 • edited Loading

Choose a reason for hiding this comment

kbenne Aug 14, 2023

Choose a reason for hiding this comment

jmarrec Aug 14, 2023

Choose a reason for hiding this comment

jmarrec commented Aug 24, 2023

kbenne commented Aug 25, 2023

jmarrec commented Aug 25, 2023 • edited Loading

kbenne commented Aug 25, 2023

ci-commercialbuildings commented Sep 27, 2023 • edited Loading

jmarrec commented Jun 23, 2023 •

edited

Loading

jmarrec Aug 14, 2023 •

edited

Loading

jmarrec Aug 14, 2023 •

edited

Loading

jmarrec commented Aug 25, 2023 •

edited

Loading

ci-commercialbuildings commented Sep 27, 2023 •

edited

Loading