-
Notifications
You must be signed in to change notification settings - Fork 222
Measure manager fixups and improvements #5304
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
Changes from all commits
16ee7a7
260ee60
2faa309
310865f
76bd106
dfc7305
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -119,7 +119,7 @@ def do_POST (request, response) | |
|
|
||
| result = {} | ||
|
|
||
| data = JSON.parse(request.body, {:symbolize_names=>true}) | ||
| data = JSON.parse(request.body || "{}", {:symbolize_names=>true}) | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor tweaks in ruby CLI. When body is empty, you don't want to get bad error on JSON.parse with an ugly backtrace and |
||
| my_measures_dir = data[:my_measures_dir] | ||
|
|
||
| if my_measures_dir | ||
|
|
@@ -132,7 +132,7 @@ def do_POST (request, response) | |
|
|
||
| result = [] | ||
|
|
||
| data = JSON.parse(request.body, {:symbolize_names=>true}) | ||
| data = JSON.parse(request.body || "{}", {:symbolize_names=>true}) | ||
| uid = data[:uid] | ||
|
|
||
| if uid | ||
|
|
@@ -156,8 +156,8 @@ def do_POST (request, response) | |
|
|
||
| result = [] | ||
|
|
||
| data = JSON.parse(request.body, {:symbolize_names=>true}) | ||
| force_reload = false | ||
| data = JSON.parse(request.body || "{}", {:symbolize_names=>true}) | ||
|
|
||
| # loop over all local BCL measures | ||
| OpenStudio::LocalBCL.instance.measures.each do |local_measure| | ||
|
|
@@ -181,7 +181,7 @@ def do_POST (request, response) | |
|
|
||
| result = [] | ||
|
|
||
| data = JSON.parse(request.body, {:symbolize_names=>true}) | ||
| data = JSON.parse(request.body || "{}", {:symbolize_names=>true}) | ||
| measures_dir = data[:measures_dir] ? data[:measures_dir] : @my_measures_dir | ||
| force_reload = data[:force_reload] ? data[:force_reload] : false | ||
|
|
||
|
|
@@ -205,7 +205,7 @@ def do_POST (request, response) | |
|
|
||
| when "/compute_arguments" | ||
|
|
||
| data = JSON.parse(request.body, {:symbolize_names=>true}) | ||
| data = JSON.parse(request.body || "{}", {:symbolize_names=>true}) | ||
| measure_dir = data[:measure_dir ] | ||
| osm_path = data[:osm_path] | ||
| force_reload = data[:force_reload] ? data[:force_reload] : false | ||
|
|
@@ -239,7 +239,7 @@ def do_POST (request, response) | |
|
|
||
| when "/create_measure" | ||
|
|
||
| data = JSON.parse(request.body, {:symbolize_names=>true}) | ||
| data = JSON.parse(request.body || "{}", {:symbolize_names=>true}) | ||
| measure_dir = data[:measure_dir] | ||
|
|
||
| # name = data[:name] # we do not take name as input | ||
|
|
@@ -263,7 +263,7 @@ def do_POST (request, response) | |
|
|
||
| when "/duplicate_measure" | ||
|
|
||
| data = JSON.parse(request.body, {:symbolize_names=>true}) | ||
| data = JSON.parse(request.body || "{}", {:symbolize_names=>true}) | ||
| old_measure_dir = data[:old_measure_dir] | ||
| measure_dir = data[:measure_dir] | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -369,6 +369,16 @@ if(BUILD_TESTING) | |
| WORKING_DIRECTORY "${PROJECT_BINARY_DIR}/Testing/" | ||
| ) | ||
|
|
||
| if (PyRequests_AVAILABLE) | ||
| add_test(NAME OpenStudioCLI.test_measure_manager | ||
| COMMAND ${Python_EXECUTABLE} -m pytest --verbose --os-cli-path $<TARGET_FILE:openstudio> "${CMAKE_CURRENT_SOURCE_DIR}/test/test_measure_manager.py" | ||
| WORKING_DIRECTORY "${PROJECT_BINARY_DIR}/Testing/" | ||
| ) | ||
| add_test(NAME OpenStudioCLI.Classic.test_measure_manager | ||
| COMMAND ${Python_EXECUTABLE} -m pytest --verbose --use-classic --os-cli-path $<TARGET_FILE:openstudio> "${CMAKE_CURRENT_SOURCE_DIR}/test/test_measure_manager.py" | ||
| WORKING_DIRECTORY "${PROJECT_BINARY_DIR}/Testing/" | ||
| ) | ||
| endif() | ||
|
Comment on lines
+372
to
+381
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Register the Classic (Ruby) and C++ version of the test_measure_manager.py |
||
|
|
||
| else() | ||
| # TODO: Remove. Fallback on these for now, as I don't know if CI has pytest installed | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| #include "../utilities/core/Filesystem.hpp" | ||
| #include "../utilities/core/FilesystemHelpers.hpp" | ||
| #include "../utilities/core/StringHelpers.hpp" | ||
| #include "../utilities/time/DateTime.hpp" | ||
| #include "../osversion/VersionTranslator.hpp" | ||
| #include "../energyplus/ForwardTranslator.hpp" | ||
| #include "../utilities/bcl/LocalBCL.hpp" | ||
|
|
@@ -145,7 +146,7 @@ Json::Value MeasureManager::internalState() const { | |
| osmInfo["checksum"] = v.checksum; | ||
| osms.append(std::move(osmInfo)); | ||
| } | ||
| result["osm"] = std::move(osms); | ||
| result["osms"] = std::move(osms); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. renamed to match Ruby |
||
|
|
||
| Json::Value idfs(Json::arrayValue); | ||
| for (const auto& [k, v] : m_idfs) { | ||
|
|
@@ -154,15 +155,18 @@ Json::Value MeasureManager::internalState() const { | |
| idfInfo["checksum"] = v.checksum; | ||
| idfs.append(std::move(idfInfo)); | ||
| } | ||
| result["idf"] = std::move(idfs); | ||
| result["idfs"] = std::move(idfs); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ruby didn't have this. But since I use |
||
|
|
||
| // Json::Value measures(Json::arrayValue); | ||
| auto& measures = result["measures"]; | ||
| measures = Json::arrayValue; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make it "[]" and not None |
||
| for (const auto& [measureDirPath, bclMeasureInfo] : m_measures) { | ||
| Json::Value mInfo(Json::objectValue); | ||
| measures.append(bclMeasureInfo.measure.toJSON()); | ||
| } | ||
|
|
||
| auto& measureInfos = result["measure_info"]; | ||
| measureInfos = Json::arrayValue; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same |
||
| // Json::Value measureInfos(Json::arrayValue); | ||
| for (const auto& [measureDirPath, bclMeasureInfo] : m_measures) { | ||
| for (const auto& [osmOrIdfPath, measureInfo] : bclMeasureInfo.measureInfos) { | ||
|
|
@@ -597,14 +601,17 @@ bool MeasureManagerServer::close() { | |
| return status == pplx::task_group_status::completed; | ||
| } | ||
|
|
||
| void MeasureManagerServer::unknown_endpoint(web::http::http_request& message) { | ||
| const std::string uri = toString(web::http::uri::decode(message.relative_uri().path())); | ||
| message.reply(web::http::status_codes::BadRequest, toWebJSON(fmt::format("Error, unknown path '{}'", uri))); | ||
| print_feedback(message, web::http::status_codes::NotFound); | ||
| } | ||
|
Comment on lines
+604
to
+608
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use a function for both GET and POST when 404 |
||
|
|
||
| void MeasureManagerServer::handle_get(web::http::http_request message) { | ||
| const std::string uri = toString(web::http::uri::decode(message.relative_uri().path())); | ||
|
|
||
| if (uri == "/") { | ||
| Json::Value result; | ||
| result["status"] = "running"; | ||
| result["my_measures_dir"] = my_measures_dir.generic_string(); | ||
| message.reply(web::http::status_codes::OK, toWebJSON(result)); | ||
| handle_request(message, web::json::value(), &MeasureManagerServer::status); | ||
| return; | ||
| } | ||
|
|
||
|
|
@@ -614,7 +621,7 @@ void MeasureManagerServer::handle_get(web::http::http_request message) { | |
| return; | ||
| } | ||
|
|
||
| message.reply(web::http::status_codes::BadRequest, toWebJSON(fmt::format("Error, unknown path '{}'", uri))); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In Get, was returning a 400 instead of a 404 when unknown endpoint |
||
| unknown_endpoint(message); | ||
| } | ||
|
|
||
| void MeasureManagerServer::handle_post(web::http::http_request message) { | ||
|
|
@@ -672,8 +679,16 @@ void MeasureManagerServer::handle_post(web::http::http_request message) { | |
| return; | ||
| } | ||
|
|
||
| message.reply(web::http::status_codes::NotFound, toWebJSON("404: Unknown Endpoint")); | ||
| unknown_endpoint(message); | ||
| } | ||
|
|
||
| MeasureManagerServer::ResponseType MeasureManagerServer::status([[maybe_unused]] const web::json::value& body) { | ||
| Json::Value result; | ||
| result["status"] = "running"; | ||
| result["my_measures_dir"] = my_measures_dir.generic_string(); | ||
| return {web::http::status_codes::OK, toWebJSON(result)}; | ||
| } | ||
|
|
||
| MeasureManagerServer::ResponseType MeasureManagerServer::internal_state([[maybe_unused]] const web::json::value& body) { | ||
| Json::Value result; | ||
| result["status"] = "running"; | ||
|
|
@@ -989,30 +1004,44 @@ MeasureManagerServer::ResponseType MeasureManagerServer::duplicate_measure(const | |
| } | ||
| } | ||
|
|
||
| void MeasureManagerServer::print_feedback(const web::http::http_request& message, web::http::status_code status_code) { | ||
| const std::string uri = toString(web::http::uri::decode(message.relative_uri().path())); | ||
| const std::string method = toString(message.method()); | ||
| const std::string http_version = message.http_version().to_utf8string(); | ||
| const std::string timestamp = openstudio::DateTime::now().toXsdDateTime(); | ||
| fmt::print(status_code == web::http::status_codes::OK ? stdout : stderr, "[{}] \"{} {} {}\" {}\n", openstudio::DateTime::now().toXsdDateTime(), | ||
| method, uri, http_version, status_code); | ||
| } | ||
|
Comment on lines
+1007
to
+1014
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Print log |
||
|
|
||
| void MeasureManagerServer::handle_request(const web::http::http_request& message, const web::json::value& body, | ||
| memRequestHandlerFunPtr request_handler) { | ||
|
|
||
| std::packaged_task<ResponseType()> task([this, &body, &request_handler]() { return (this->*request_handler)(body); }); | ||
|
|
||
| auto future_result = task.get_future(); // The task hasn't been started yet | ||
| tasks.push_back(std::move(task)); // It gets queued, the **main** thread will process it | ||
| web::http::status_code status_code = web::http::status_codes::Created; | ||
| try { | ||
| auto result = future_result.get(); // This block until it's been processed | ||
| message.reply(result.status_code, result.body); | ||
| status_code = result.status_code; | ||
| message.reply(status_code, result.body); | ||
| } catch (const std::exception& e) { | ||
| constexpr auto msg = "MeasureManager Server encountered an error:\n\"{}\"\n"; | ||
| fmt::print(msg, e.what()); | ||
| status_code = web::http::status_codes::InternalError; | ||
| message.reply(web::http::status_codes::InternalError, fmt::format(msg, e.what())); | ||
| } | ||
| print_feedback(message, status_code); | ||
| } | ||
|
|
||
| void MeasureManagerServer::do_tasks_forever() { | ||
| fmt::print("MeasureManager Ready"); | ||
| fmt::print("MeasureManager Ready\n"); | ||
| fmt::print("Accepting requests on: {}\n", m_url); | ||
| std::fflush(stdout); | ||
| while (true) { | ||
| auto task = tasks.wait_for_one(); | ||
| task(); | ||
| std::fflush(stdout); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,11 +12,16 @@ def validate_file(arg): | |
|
|
||
| def pytest_addoption(parser): | ||
| parser.addoption("--os-cli-path", type=validate_file, help="Path to the OS CLI") # , required=True | ||
|
|
||
| parser.addoption("--use-classic", action="store_true", help="Force use the Classic CLI") | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. need addopt |
||
|
|
||
| @pytest.fixture(scope="module") | ||
| def osclipath(request): | ||
| cli_path = request.config.getoption("--os-cli-path") | ||
| if cli_path is None: | ||
| raise ValueError("You must supply --os-cli-path [Path]") | ||
| return cli_path | ||
|
|
||
| @pytest.fixture(scope="module") | ||
| def use_classic_cli(request): | ||
| use_classic = request.config.getoption("--use-classic") | ||
| return use_classic | ||
|
Comment on lines
+23
to
+27
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Set it as a fixture for use |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Detect if
requestsis installed on the system python