From 5d301d8e4df4448ee9c3a403b60ff79d1964034a Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Thu, 7 Nov 2024 11:30:47 +0100 Subject: [PATCH 1/7] Prepare a few broken OSWs for testing * One references a non existing measure * One references a folder that can be found but isn't a valid BCLMeasure * One has a ReportingMeasure before a ModelMeasure --- .../invalid_measures/missing_a_measure.osw | 19 +++++++++++++++++++ .../invalid_measures/unloadable_measure.osw | 19 +++++++++++++++++++ .../wrong_measure_type_order.osw | 15 +++++++++++++++ .../measures/UnloadableMeasure/README.md | 1 + .../measures/UnloadableMeasure/measure.rb | 0 5 files changed, 54 insertions(+) create mode 100644 resources/workflow/invalid_measures/missing_a_measure.osw create mode 100644 resources/workflow/invalid_measures/unloadable_measure.osw create mode 100644 resources/workflow/invalid_measures/wrong_measure_type_order.osw create mode 100644 resources/workflow/measures/UnloadableMeasure/README.md create mode 100644 resources/workflow/measures/UnloadableMeasure/measure.rb diff --git a/resources/workflow/invalid_measures/missing_a_measure.osw b/resources/workflow/invalid_measures/missing_a_measure.osw new file mode 100644 index 00000000000..9de282f47a0 --- /dev/null +++ b/resources/workflow/invalid_measures/missing_a_measure.osw @@ -0,0 +1,19 @@ +{ + "weather_file": "../../Examples/compact_osw/files/srrl_2013_amy.epw", + "seed_file": "../example_model.osm", + "measure_paths": ["../measures/"], + "steps": [ + { + "measure_dir_name": "ModelMeasureRegistersError", + "arguments": {} + }, + { + "measure_dir_name": "NON_EXISTING_MEASURE_THIS_SHOULD_BE_CAUGHT", + "arguments": {} + }, + { + "measure_dir_name": "FakeReport", + "arguments": {} + } + ] +} diff --git a/resources/workflow/invalid_measures/unloadable_measure.osw b/resources/workflow/invalid_measures/unloadable_measure.osw new file mode 100644 index 00000000000..ebee30c7e60 --- /dev/null +++ b/resources/workflow/invalid_measures/unloadable_measure.osw @@ -0,0 +1,19 @@ +{ + "weather_file": "../../Examples/compact_osw/files/srrl_2013_amy.epw", + "seed_file": "../example_model.osm", + "measure_paths": ["../measures/"], + "steps": [ + { + "measure_dir_name": "ModelMeasureRegistersError", + "arguments": {} + }, + { + "measure_dir_name": "UnloadableMeasure", + "arguments": {} + }, + { + "measure_dir_name": "FakeReport", + "arguments": {} + } + ] +} diff --git a/resources/workflow/invalid_measures/wrong_measure_type_order.osw b/resources/workflow/invalid_measures/wrong_measure_type_order.osw new file mode 100644 index 00000000000..710cfd4e345 --- /dev/null +++ b/resources/workflow/invalid_measures/wrong_measure_type_order.osw @@ -0,0 +1,15 @@ +{ + "weather_file": "../../Examples/compact_osw/files/srrl_2013_amy.epw", + "seed_file": "../example_model.osm", + "measure_paths": ["../measures/"], + "steps": [ + { + "measure_dir_name": "FakeReport", + "arguments": {} + }, + { + "measure_dir_name": "ModelMeasureRegistersError", + "arguments": {} + } + ] +} diff --git a/resources/workflow/measures/UnloadableMeasure/README.md b/resources/workflow/measures/UnloadableMeasure/README.md new file mode 100644 index 00000000000..4459d7e1aa3 --- /dev/null +++ b/resources/workflow/measures/UnloadableMeasure/README.md @@ -0,0 +1 @@ +This doesnt have a xml diff --git a/resources/workflow/measures/UnloadableMeasure/measure.rb b/resources/workflow/measures/UnloadableMeasure/measure.rb new file mode 100644 index 00000000000..e69de29bb2d From c5481ef5d52d84912ad0f1d6b2a9d3a615d2a6e9 Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Thu, 7 Nov 2024 13:14:27 +0100 Subject: [PATCH 2/7] Don't use a measure that register an Error in the broken OSW (so they DO run with current C++ CLI to show problem) --- .../invalid_measures/missing_a_measure.osw | 2 +- .../invalid_measures/unloadable_measure.osw | 2 +- .../wrong_measure_type_order.osw | 2 +- .../measures/FakeModelMeasure/measure.rb | 42 ++++++++++++++++++ .../measures/FakeModelMeasure/measure.xml | 44 +++++++++++++++++++ 5 files changed, 89 insertions(+), 3 deletions(-) create mode 100644 resources/workflow/measures/FakeModelMeasure/measure.rb create mode 100644 resources/workflow/measures/FakeModelMeasure/measure.xml diff --git a/resources/workflow/invalid_measures/missing_a_measure.osw b/resources/workflow/invalid_measures/missing_a_measure.osw index 9de282f47a0..2f78c0633ba 100644 --- a/resources/workflow/invalid_measures/missing_a_measure.osw +++ b/resources/workflow/invalid_measures/missing_a_measure.osw @@ -4,7 +4,7 @@ "measure_paths": ["../measures/"], "steps": [ { - "measure_dir_name": "ModelMeasureRegistersError", + "measure_dir_name": "FakeModelMeasure", "arguments": {} }, { diff --git a/resources/workflow/invalid_measures/unloadable_measure.osw b/resources/workflow/invalid_measures/unloadable_measure.osw index ebee30c7e60..cb5d1c88178 100644 --- a/resources/workflow/invalid_measures/unloadable_measure.osw +++ b/resources/workflow/invalid_measures/unloadable_measure.osw @@ -4,7 +4,7 @@ "measure_paths": ["../measures/"], "steps": [ { - "measure_dir_name": "ModelMeasureRegistersError", + "measure_dir_name": "FakeModelMeasure", "arguments": {} }, { diff --git a/resources/workflow/invalid_measures/wrong_measure_type_order.osw b/resources/workflow/invalid_measures/wrong_measure_type_order.osw index 710cfd4e345..01b28e49177 100644 --- a/resources/workflow/invalid_measures/wrong_measure_type_order.osw +++ b/resources/workflow/invalid_measures/wrong_measure_type_order.osw @@ -8,7 +8,7 @@ "arguments": {} }, { - "measure_dir_name": "ModelMeasureRegistersError", + "measure_dir_name": "FakeModelMeasure", "arguments": {} } ] diff --git a/resources/workflow/measures/FakeModelMeasure/measure.rb b/resources/workflow/measures/FakeModelMeasure/measure.rb new file mode 100644 index 00000000000..68a978a9096 --- /dev/null +++ b/resources/workflow/measures/FakeModelMeasure/measure.rb @@ -0,0 +1,42 @@ +class FakeModelMeasure < OpenStudio::Measure::ModelMeasure + # human readable name + def name + # Measure name should be the title case of the class name. + return 'A dumb ModelMeasure' + end + + # human readable description + def description + return 'Does nothing' + end + + # human readable description of modeling approach + def modeler_description + return 'Just for testing' + end + + # define the arguments that the user will input + def arguments(model) + args = OpenStudio::Measure::OSArgumentVector.new + + return args + end + + # define what happens when the measure is run + def run(model, runner, user_arguments) + super(model, runner, user_arguments) # Do **NOT** remove this line + + # use the built-in error checking + if !runner.validateUserArguments(arguments(model), user_arguments) + return false + end + + # report final condition of model + runner.registerFinalCondition("The FakeModelMeasure run.") + + return true + end +end + +# register the measure to be used by the application +FakeModelMeasure.new.registerWithApplication diff --git a/resources/workflow/measures/FakeModelMeasure/measure.xml b/resources/workflow/measures/FakeModelMeasure/measure.xml new file mode 100644 index 00000000000..a1fd30f66f0 --- /dev/null +++ b/resources/workflow/measures/FakeModelMeasure/measure.xml @@ -0,0 +1,44 @@ + + + 3.1 + fake_model_measure + 677b8fd3-2627-4516-b090-f6e47dc99fea + 162465e5-b6f6-419f-ab5f-c2fc4bd549e0 + 2024-11-07T11:55:10Z + 82D8F881 + FakeModelMeasure + A dumb ModelMeasure + Does nothing + Just for testing + + + + + Envelope.Form + + + + Measure Type + ModelMeasure + string + + + Measure Language + Ruby + string + + + + + + OpenStudio + 3.9.0 + 3.9.0 + + measure.rb + rb + script + DDA977B0 + + + From 6729e7684f9666779ac91e61d97dab5d08123010 Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Thu, 7 Nov 2024 13:14:56 +0100 Subject: [PATCH 3/7] Add CLI tests with the broken OSWs, expect them to fail (they don't with current C++ CLI) --- src/cli/CMakeLists.txt | 57 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/src/cli/CMakeLists.txt b/src/cli/CMakeLists.txt index 179c35f7a04..e79684fcdfc 100644 --- a/src/cli/CMakeLists.txt +++ b/src/cli/CMakeLists.txt @@ -270,6 +270,63 @@ if(BUILD_TESTING) PASS_REGULAR_EXPRESSION "HI FROM ERB PYTHON PLUGIN[\r\n\t ]*HI FROM JINJA PYTHON PLUGIN" ) + # ======================== Workflows should fail ======================== + add_test(NAME OpenStudioCLI.Run_Validate.MissingAMeasure + COMMAND $ run --show-stdout -w missing_a_measure.osw + WORKING_DIRECTORY "${PROJECT_BINARY_DIR}/resources/workflow/invalid_measures/" + ) + set_tests_properties(OpenStudioCLI.Run_Validate.MissingAMeasure PROPERTIES + WILL_FAIL TRUE + RESOURCE_LOCK "invalid_measures" + ) + + add_test(NAME OpenStudioCLI.Run_Validate.UnloadableMeasure + COMMAND $ run --show-stdout -w unloadable_measure.osw + WORKING_DIRECTORY "${PROJECT_BINARY_DIR}/resources/workflow/invalid_measures/" + ) + set_tests_properties(OpenStudioCLI.Run_Validate.UnloadableMeasure PROPERTIES + WILL_FAIL TRUE + RESOURCE_LOCK "invalid_measures" + ) + + add_test(NAME OpenStudioCLI.Run_Validate.WrongMeasureTypeOrder + COMMAND $ run --show-stdout -w wrong_measure_type_order.osw + WORKING_DIRECTORY "${PROJECT_BINARY_DIR}/resources/workflow/invalid_measures/" + ) + set_tests_properties(OpenStudioCLI.Run_Validate.WrongMeasureTypeOrder PROPERTIES + WILL_FAIL TRUE + RESOURCE_LOCK "invalid_measures" + ) + + # Classic + add_test(NAME OpenStudioCLI.Classic.Run_Validate.MissingAMeasure + COMMAND $ classic run --show-stdout -w missing_a_measure.osw + WORKING_DIRECTORY "${PROJECT_BINARY_DIR}/resources/workflow/invalid_measures/" + ) + set_tests_properties(OpenStudioCLI.Classic.Run_Validate.MissingAMeasure PROPERTIES + WILL_FAIL TRUE + RESOURCE_LOCK "invalid_measures" + ) + + add_test(NAME OpenStudioCLI.Classic.Run_Validate.UnloadableMeasure + COMMAND $ classic run --show-stdout -w unloadable_measure.osw + WORKING_DIRECTORY "${PROJECT_BINARY_DIR}/resources/workflow/invalid_measures/" + ) + set_tests_properties(OpenStudioCLI.Classic.Run_Validate.UnloadableMeasure PROPERTIES + WILL_FAIL TRUE + RESOURCE_LOCK "invalid_measures" + ) + + add_test(NAME OpenStudioCLI.Classic.Run_Validate.WrongMeasureTypeOrder + COMMAND $ classic run --show-stdout -w wrong_measure_type_order.osw + WORKING_DIRECTORY "${PROJECT_BINARY_DIR}/resources/workflow/invalid_measures/" + ) + set_tests_properties(OpenStudioCLI.Classic.Run_Validate.WrongMeasureTypeOrder PROPERTIES + WILL_FAIL TRUE + RESOURCE_LOCK "invalid_measures" + ) + # ====================== End Workflows should fail ====================== + if (Pytest_AVAILABLE) add_test(NAME OpenStudioCLI.test_loglevel COMMAND ${Python_EXECUTABLE} -m pytest --verbose ${Pytest_XDIST_OPTS} --os-cli-path $ "${CMAKE_CURRENT_SOURCE_DIR}/test/test_loglevel.py" From b249fef94ea0ef6d6410367ef761abbd3b83d38d Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Thu, 7 Nov 2024 11:32:25 +0100 Subject: [PATCH 4/7] Add `bool WorkflowJSON::validateMeasures() const`: non-throwy, logs Error messages TODO: should we return early? --- src/utilities/filetypes/WorkflowJSON.cpp | 63 +++++++++++++++++++ src/utilities/filetypes/WorkflowJSON.hpp | 3 + src/utilities/filetypes/WorkflowJSON_Impl.hpp | 2 + 3 files changed, 68 insertions(+) diff --git a/src/utilities/filetypes/WorkflowJSON.cpp b/src/utilities/filetypes/WorkflowJSON.cpp index 1ba116ceea1..076fa823d85 100644 --- a/src/utilities/filetypes/WorkflowJSON.cpp +++ b/src/utilities/filetypes/WorkflowJSON.cpp @@ -851,6 +851,65 @@ namespace detail { } } + bool WorkflowJSON_Impl::validateMeasures() const { + // TODO: should we exit early, or return all problems found? + + bool result = true; + MeasureType state = MeasureType::ModelMeasure; + + for (size_t i = 0; const auto& step : m_steps) { + LOG(Debug, "Validating step " << i); + if (auto step_ = step.optionalCast()) { + // Not calling getBCLMeasure because I want to mimic workflow-gem and be as explicit as possible about what went wrong + const auto measureDirName = step_->measureDirName(); + auto measurePath_ = findMeasure(measureDirName); + if (!measurePath_) { + LOG(Error, "Cannot find measure '" << measureDirName << "'"); + result = false; + continue; + } + auto bclMeasure_ = BCLMeasure::load(*measurePath_); + if (!bclMeasure_) { + LOG(Error, "Cannot load measure '" << measureDirName << "' at '" << *measurePath_ << "'"); + result = false; + continue; + } + + // Ensure that measures are in order, i.e. no OS after E+, E+ or OS after Reporting + const auto measureType = bclMeasure_->measureType(); + + if (measureType == MeasureType::ModelMeasure) { + if (state == MeasureType::EnergyPlusMeasure) { + LOG(Error, "OpenStudio measure '" << measureDirName << "' called after transition to EnergyPlus."); + result = false; + } + if (state == MeasureType::ReportingMeasure) { + LOG(Error, "OpenStudio measure '" << measureDirName << "' called after Energyplus simulation."); + result = false; + } + + } else if (measureType == MeasureType::EnergyPlusMeasure) { + if (state == MeasureType::ReportingMeasure) { + LOG(Error, "EnergyPlus measure '" << measureDirName << "' called after Energyplus simulation."); + result = false; + } + if (state == MeasureType::ModelMeasure) { + state = MeasureType::EnergyPlusMeasure; + } + + } else if (measureType == MeasureType::ReportingMeasure) { + state = MeasureType::ReportingMeasure; + + } else { + LOG(Error, "MeasureType " << measureType.valueName() << " of measure '" << measureDirName << "' is not supported"); + result = false; + } + } + ++i; + } + + return result; + } } // namespace detail WorkflowJSON::WorkflowJSON() : m_impl(std::shared_ptr(new detail::WorkflowJSON_Impl())) {} @@ -1123,6 +1182,10 @@ void WorkflowJSON::resetRunOptions() { getImpl()->resetRunOptions(); } +bool WorkflowJSON::validateMeasures() const { + return getImpl()->validateMeasures(); +} + std::ostream& operator<<(std::ostream& os, const WorkflowJSON& workflowJSON) { os << workflowJSON.string(); return os; diff --git a/src/utilities/filetypes/WorkflowJSON.hpp b/src/utilities/filetypes/WorkflowJSON.hpp index 1ac097baa7f..5d6c4dd693b 100644 --- a/src/utilities/filetypes/WorkflowJSON.hpp +++ b/src/utilities/filetypes/WorkflowJSON.hpp @@ -233,6 +233,9 @@ class UTILITIES_API WorkflowJSON /** Reset RunOptions for this workflow. */ void resetRunOptions(); + /** Checks that all measures in the Workflow can be found, and are in the correct order (ModelMeasure > EnergyPlusMeasure > ReportingMeasure) */ + bool validateMeasures() const; + protected: // get the impl template diff --git a/src/utilities/filetypes/WorkflowJSON_Impl.hpp b/src/utilities/filetypes/WorkflowJSON_Impl.hpp index 65d65c52e83..03be7a07339 100644 --- a/src/utilities/filetypes/WorkflowJSON_Impl.hpp +++ b/src/utilities/filetypes/WorkflowJSON_Impl.hpp @@ -153,6 +153,8 @@ namespace detail { // Emitted on any change Nano::Signal onChange; + bool validateMeasures() const; + private: REGISTER_LOGGER("openstudio.WorkflowJSON"); From dd6a610dc60b9487ddfaf3649b74cae4b6def834 Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Thu, 7 Nov 2024 11:33:16 +0100 Subject: [PATCH 5/7] Use that in the RunInitialization job, and throw if invalid --- src/workflow/RunInitialization.cpp | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/workflow/RunInitialization.cpp b/src/workflow/RunInitialization.cpp index a35ba499a64..0d691b2dba5 100644 --- a/src/workflow/RunInitialization.cpp +++ b/src/workflow/RunInitialization.cpp @@ -54,10 +54,17 @@ void OSWorkflow::runInitialization() { } }); - // TODO: create the runner with our WorkflowJSON (workflow gem uses datapoint/analysis too?!) - // TODO: Validate the OSW measures if the flag is set to true, (the default state) - // Note JM 2022-11-07: Is it better to try and load all measures once, instead of crashing later? + // There isn't a 'verify_osw' key in the RunOptions, so always do it for now. Maybe don't if `fast`? + { + LOG(Info, "Attempting to validate the measure workflow"); + + if (!workflowJSON.validateMeasures()) { + LOG_AND_THROW("Workflow is invalid"); + } + + LOG(Info, "Validated the measure workflow"); + } LOG(Debug, "Finding and loading the seed file"); auto seedPath_ = workflowJSON.seedFile(); From 290661bd6f95663ab534e7a09888d07b1faa9f0d Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Thu, 7 Nov 2024 12:25:09 +0100 Subject: [PATCH 6/7] Test the new method Forgot to update the Ctest after using another measure --- .../filetypes/test/WorkflowJSON_GTest.cpp | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/src/utilities/filetypes/test/WorkflowJSON_GTest.cpp b/src/utilities/filetypes/test/WorkflowJSON_GTest.cpp index d88b5b8247a..48e2eb5a070 100644 --- a/src/utilities/filetypes/test/WorkflowJSON_GTest.cpp +++ b/src/utilities/filetypes/test/WorkflowJSON_GTest.cpp @@ -15,9 +15,11 @@ #include "../../time/DateTime.hpp" +#include "../../core/Filesystem.hpp" #include "../../core/Exception.hpp" #include "../../core/System.hpp" #include "../../core/Checksum.hpp" +#include "../../core/StringStreamLogSink.hpp" #include @@ -1436,3 +1438,48 @@ TEST(Filetypes, RunOptions_overrideValuesWith) { ASSERT_FALSE(ftOptions.excludeSpaceTranslation()); ASSERT_TRUE(ftOptions.isExcludeSpaceTranslationDefaulted()); } + +TEST(Filetypes, WorkflowJSON_ValidateMeasures_Ok) { + auto p = resourcesPath() / toPath("utilities/Filetypes/full.osw"); + WorkflowJSON w(p); + EXPECT_TRUE(w.validateMeasures()); +} + +TEST(Filetypes, WorkflowJSON_ValidateMeasures_Missing) { + auto p = resourcesPath() / toPath("workflow/invalid_measures/missing_a_measure.osw"); + ASSERT_TRUE(boost::filesystem::is_regular_file(p)); + WorkflowJSON w(p); + StringStreamLogSink sink; + sink.setLogLevel(Error); + EXPECT_FALSE(w.validateMeasures()); + ASSERT_EQ(1, sink.logMessages().size()); + EXPECT_EQ("Cannot find measure 'NON_EXISTING_MEASURE_THIS_SHOULD_BE_CAUGHT'", sink.logMessages()[0].logMessage()); +} + +TEST(Filetypes, WorkflowJSON_ValidateMeasures_Unloadable) { + auto p = resourcesPath() / toPath("workflow/invalid_measures/unloadable_measure.osw"); + ASSERT_TRUE(boost::filesystem::is_regular_file(p)); + WorkflowJSON w(p); + StringStreamLogSink sink; + sink.setLogLevel(Error); + EXPECT_FALSE(w.validateMeasures()); + auto logMessages = sink.logMessages(); + ASSERT_EQ(3, logMessages.size()); + EXPECT_EQ("utilities.bcl.BCLXML", logMessages.at(0).logChannel()); + EXPECT_EQ("utilities.bcl.BCLMeasure", logMessages.at(1).logChannel()); + EXPECT_EQ("openstudio.WorkflowJSON", logMessages.at(2).logChannel()); + auto logMessage = sink.logMessages()[2].logMessage(); + EXPECT_TRUE(logMessage.find("Cannot load measure 'UnloadableMeasure' at '") != std::string::npos) << logMessage; +} + +TEST(Filetypes, WorkflowJSON_ValidateMeasures_WrongOrder) { + auto p = resourcesPath() / toPath("workflow/invalid_measures/wrong_measure_type_order.osw"); + ASSERT_TRUE(boost::filesystem::is_regular_file(p)); + WorkflowJSON w(p); + StringStreamLogSink sink; + sink.setLogLevel(Error); + EXPECT_FALSE(w.validateMeasures()); + ASSERT_EQ(1, sink.logMessages().size()); + + EXPECT_EQ("OpenStudio measure 'FakeModelMeasure' called after Energyplus simulation.", sink.logMessages()[0].logMessage()); +} From 4aafb5ab3f3c5087d675f388cf4fb87e4f647785 Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Thu, 7 Nov 2024 12:27:57 +0100 Subject: [PATCH 7/7] When loading from a string that is actually a path, call setOswPath --- src/utilities/filetypes/WorkflowJSON.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/utilities/filetypes/WorkflowJSON.cpp b/src/utilities/filetypes/WorkflowJSON.cpp index 076fa823d85..4bbc8a60393 100644 --- a/src/utilities/filetypes/WorkflowJSON.cpp +++ b/src/utilities/filetypes/WorkflowJSON.cpp @@ -46,10 +46,12 @@ namespace detail { std::string formattedErrors; bool parsingSuccessful = Json::parseFromStream(rbuilder, ss, &m_value, &formattedErrors); + openstudio::path p; + if (!parsingSuccessful) { // see if this is a path - openstudio::path p = toPath(s); + p = toPath(s); if (boost::filesystem::exists(p) && boost::filesystem::is_regular_file(p)) { // open file std::ifstream ifs(openstudio::toSystemFilename(p)); @@ -65,6 +67,10 @@ namespace detail { parseSteps(); parseRunOptions(); + + if (!p.empty()) { + setOswPath(p, false); + } } WorkflowJSON_Impl::WorkflowJSON_Impl(const openstudio::path& p) {