Fix #5233 - Validate OSW measures before running #5295

jmarrec · 2024-11-07T12:21:21Z

Pull request overview

Fixes running osw when measure not found, instead of stopping it keeps running the osw and E+. #5233

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

jmarrec · 2024-11-07T12:21:48Z

resources/workflow/invalid_measures/missing_a_measure.osw

+			"measure_dir_name": "NON_EXISTING_MEASURE_THIS_SHOULD_BE_CAUGHT",
+			"arguments": {}


jmarrec · 2024-11-07T12:22:08Z

resources/workflow/invalid_measures/unloadable_measure.osw

+			"measure_dir_name": "UnloadableMeasure",
+			"arguments": {}


This is an existing directory (findMeasure works), but it doesn't have a measure.xml

jmarrec · 2024-11-07T12:22:19Z

resources/workflow/invalid_measures/wrong_measure_type_order.osw

+		{
+			"measure_dir_name": "FakeReport",
+			"arguments": {}
+		},
+		{
+			"measure_dir_name": "FakeModelMeasure",
+			"arguments": {}
+		}


Wrong order

jmarrec · 2024-11-07T12:22:49Z

src/cli/CMakeLists.txt

+  # ======================== Workflows should fail ========================
+  add_test(NAME OpenStudioCLI.Run_Validate.MissingAMeasure
+    COMMAND $<TARGET_FILE:openstudio> 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 $<TARGET_FILE:openstudio> 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 $<TARGET_FILE:openstudio> 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 $<TARGET_FILE:openstudio> 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 $<TARGET_FILE:openstudio> 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 $<TARGET_FILE:openstudio> 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 ======================


Add a c++ and a classic (ruby) version of the calling the CLI on these broken workflows. Expect all of them to fail

I intently pushed just these tests for now, so that CI would report a test failure and highlight the fact that these correctly test the issue #5233

3798 - OpenStudioCLI.Run_Validate.MissingAMeasure (Failed) 3799 - OpenStudioCLI.Run_Validate.UnloadableMeasure (Failed) 3800 - OpenStudioCLI.Run_Validate.WrongMeasureTypeOrder (Failed)

jmarrec · 2024-11-07T12:37:42Z

src/utilities/filetypes/WorkflowJSON.hpp

+  /** Checks that all measures in the Workflow can be found, and are in the correct order (ModelMeasure > EnergyPlusMeasure > ReportingMeasure) */
+  bool validateMeasures() const;


new WorkflowJSON method. Non-throwy, logs Errors and return a bool

jmarrec · 2024-11-07T12:41:42Z

src/utilities/filetypes/WorkflowJSON.cpp

+    if (!p.empty()) {
+      setOswPath(p, false);
+    }


In the std::string& constructor, if it happens to be a path, do call setOswPath like we do for the ctor that takes a openstudio::path.

In Ruby, that means on develop you have a different behavior between these two!

OpenStudio::WorkflowJSON.new("/path/to/workflow.osw") OpenStudio::WorkflowJSON.new(OpenStudio::toPath("/path/to/workflow.osw"))

I noticed this because if the oswPath isn't set, the setMeasuresTypes (private, impl only) isn't called, and if you try to call workflow.getMeasureStepsWithIndex or getMeasureSteps you hit an OS_ASSERT

OpenStudio/src/utilities/filetypes/WorkflowJSON.cpp

Line 607 in cc1e0bb

OS_ASSERT(m_measureTypes.size() == n);

I think that's pretty broken, but it's outside of the scope of this PR to fix it.

As long as we document this to fix later that's good

I know it might seem a bit backwards, but perhaps the openstudio::path version of the constructor should just forward the call to the string version.

As it is there seems to be an underiable opportunity to have different behaviors between the two constructors, since the path version has its own implementation.

Meh. You can guess why it's the way it is.

The string constructor was really only meant to take an actual JSON string in originally

The path& one was to read from a file on disk

People using ruby especially would try to do OpenStudio::WorkflowJSON.new("/my/workflow.osw") (instead of OpenStudio::WorkflowJSON.new(OpenStudio::toPath("/my/workflow.osw")) and would be very confused, so instead the string version when it doesn't parse correctly, it falls back to check if the string argument is actually a path, and if so I'm pretty sure the intent was to do exactly the same thing. C++ constructor chaining is not possible, so instead of bothering to make an internal method fromPath(...) for a couple of lines, the path version was copy pasted in. This happened in a7efe66 , and probably just forgot the setOswPath since the flow control (with parseSteps etc) was different.

jmarrec · 2024-11-07T12:42:20Z

src/utilities/filetypes/WorkflowJSON.cpp

+  bool WorkflowJSON_Impl::validateMeasures() const {
+    // TODO: should we exit early, or return all problems found?


@DavidGoldwasser @kbenne thoughts here? I went with the "Find all problems" approach.

This mimics this workflow-gem validate_measures method: https://github.com/NREL/OpenStudio-workflow-gem/blob/39c84e45f152446a92379a501807e2691dba15ab/lib/openstudio/workflow/util/measure.rb#L97-L151

I like the find all problems approach and I think that is what most people will want.

Outside of the scope here, but I'm thinking that perhaps "validation" would be stronger if we actually loaded the measure script into the Python/Ruby engine. Unless I'm mistaken, as it stands, I'm pretty sure that is not happening here. We have encountered some issues (like this) where things go sideways, yet the root cause is that the measure syntax is flawed, but the failure mode misleads the user to thinking there might be an issue within OpenStudio itself.

The rationale of catching this early is perhaps sound, but it's also slow. Not sure what's better.

jmarrec · 2024-11-07T12:43:28Z

src/utilities/filetypes/WorkflowJSON.cpp

+    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<MeasureStep>()) {
+        // 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;
+        }


For each step, check we can find the measure directory, and it's a valid BCL Measure.

jmarrec · 2024-11-07T12:43:47Z

src/utilities/filetypes/WorkflowJSON.cpp

+        // 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;


And ensure the measures are in the right order

jmarrec · 2024-11-07T12:45:03Z

src/workflow/RunInitialization.cpp

  // 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");
+  }


Same as https://github.com/NREL/OpenStudio-workflow-gem/blob/39c84e45f152446a92379a501807e2691dba15ab/lib/openstudio/workflow/jobs/run_initialization.rb#L89-L94

TODO:

Should verify_osw be added to the OSW Schema and to RunOptions?

Should we maybe not validate it if fast is passed? @shorowit might have opinions on this

jmarrec · 2024-11-07T12:45:23Z

src/utilities/filetypes/test/WorkflowJSON_GTest.cpp

+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 'ModelMeasureRegistersError' called after Energyplus simulation.", sink.logMessages()[0].logMessage());
+}


Gtests also added

* 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

… DO run with current C++ CLI to show problem)

…ith current C++ CLI)

…rror messages TODO: should we return early?

Forgot to update the Ctest after using another measure

ci-commercialbuildings · 2024-11-07T14:17:22Z

CI Results for 4aafb5a:

Ubuntu 24.04:
- OpenStudio-3.9.0-rc2+4aafb5ab3f-Ubuntu-24.04-x86_64.deb
- OpenStudio-3.9.0-rc2+4aafb5ab3f-Ubuntu-24.04-x86_64.tar.gz
Ubuntu 22.04:
Darwin:
- OpenStudio-3.9.0-rc2+4aafb5ab3f-Darwin-x86_64.dmg
- OpenStudio-3.9.0-rc2+4aafb5ab3f-Darwin-x86_64.tar.gz
Windows:
- OpenStudio-3.9.0-rc2+4aafb5ab3f-Windows.exe
- OpenStudio-3.9.0-rc2+4aafb5ab3f-Windows.tar.gz
Code Coverage:

kbenne · 2024-11-11T19:21:47Z

src/utilities/filetypes/WorkflowJSON.cpp

+    if (!p.empty()) {
+      setOswPath(p, false);
+    }


I know it might seem a bit backwards, but perhaps the openstudio::path version of the constructor should just forward the call to the string version.

kbenne · 2024-11-11T19:23:00Z

src/utilities/filetypes/WorkflowJSON.cpp

+    if (!p.empty()) {
+      setOswPath(p, false);
+    }


As it is there seems to be an underiable opportunity to have different behaviors between the two constructors, since the path version has its own implementation.

kbenne · 2024-11-11T19:31:02Z

src/utilities/filetypes/WorkflowJSON.cpp

+  bool WorkflowJSON_Impl::validateMeasures() const {
+    // TODO: should we exit early, or return all problems found?


I like the find all problems approach and I think that is what most people will want.

Outside of the scope here, but I'm thinking that perhaps "validation" would be stronger if we actually loaded the measure script into the Python/Ruby engine. Unless I'm mistaken, as it stands, I'm pretty sure that is not happening here. We have encountered some issues (like this) where things go sideways, yet the root cause is that the measure syntax is flawed, but the failure mode misleads the user to thinking there might be an issue within OpenStudio itself.

kbenne · 2024-11-11T19:32:11Z

I'm going to go ahead merge, but I left a few discussion points.

jmarrec added component - Workflow Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. labels Nov 7, 2024

jmarrec self-assigned this Nov 7, 2024

jmarrec commented Nov 7, 2024

View reviewed changes

jmarrec force-pushed the 5233_ValidateOSW branch from a3c8f24 to df988e8 Compare November 7, 2024 12:37

jmarrec commented Nov 7, 2024

View reviewed changes

jmarrec requested review from kbenne and DavidGoldwasser November 7, 2024 12:47

jmarrec added 7 commits November 7, 2024 14:44

Prepare a few broken OSWs for testing

5d301d8

* 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

Don't use a measure that register an Error in the broken OSW (so they…

c5481ef

… DO run with current C++ CLI to show problem)

Add CLI tests with the broken OSWs, expect them to fail (they don't w…

6729e76

…ith current C++ CLI)

Add bool WorkflowJSON::validateMeasures() const: non-throwy, logs E…

b249fef

…rror messages TODO: should we return early?

Use that in the RunInitialization job, and throw if invalid

dd6a610

Test the new method

290661b

Forgot to update the Ctest after using another measure

When loading from a string that is actually a path, call setOswPath

4aafb5a

jmarrec force-pushed the 5233_ValidateOSW branch from df988e8 to 4aafb5a Compare November 7, 2024 13:44

DavidGoldwasser approved these changes Nov 8, 2024

View reviewed changes

DavidGoldwasser added this to the OpenStudio SDK 3.9.0 milestone Nov 8, 2024

kbenne approved these changes Nov 11, 2024

View reviewed changes

kbenne merged commit 8cccf80 into develop Nov 11, 2024
5 of 6 checks passed

kbenne deleted the 5233_ValidateOSW branch November 11, 2024 19:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #5233 - Validate OSW measures before running #5295

Fix #5233 - Validate OSW measures before running #5295

jmarrec commented Nov 7, 2024

jmarrec Nov 7, 2024

jmarrec Nov 7, 2024

jmarrec Nov 7, 2024

jmarrec Nov 7, 2024

jmarrec Nov 7, 2024

jmarrec Nov 7, 2024

jmarrec Nov 7, 2024

DavidGoldwasser Nov 11, 2024

kbenne Nov 11, 2024

kbenne Nov 11, 2024

jmarrec Nov 12, 2024

jmarrec Nov 7, 2024

jmarrec Nov 7, 2024

kbenne Nov 11, 2024

jmarrec Nov 12, 2024

jmarrec Nov 7, 2024

jmarrec Nov 7, 2024

jmarrec Nov 7, 2024

jmarrec Nov 7, 2024

ci-commercialbuildings commented Nov 7, 2024 •

edited

Loading

kbenne Nov 11, 2024

kbenne Nov 11, 2024

kbenne Nov 11, 2024

kbenne commented Nov 11, 2024

		"measure_dir_name": "NON_EXISTING_MEASURE_THIS_SHOULD_BE_CAUGHT",
		"arguments": {}

		/** Checks that all measures in the Workflow can be found, and are in the correct order (ModelMeasure > EnergyPlusMeasure > ReportingMeasure) */
		bool validateMeasures() const;

		bool WorkflowJSON_Impl::validateMeasures() const {
		// TODO: should we exit early, or return all problems found?

Fix #5233 - Validate OSW measures before running #5295

Fix #5233 - Validate OSW measures before running #5295

Conversation

jmarrec commented Nov 7, 2024

Pull request overview

Pull Request Author

Review Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ci-commercialbuildings commented Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbenne commented Nov 11, 2024

ci-commercialbuildings commented Nov 7, 2024 •

edited

Loading