Fix #5233 - Validate OSW measures before running #5295
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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": "FakeModelMeasure", | ||
| "arguments": {} | ||
| }, | ||
| { | ||
| "measure_dir_name": "NON_EXISTING_MEASURE_THIS_SHOULD_BE_CAUGHT", | ||
| "arguments": {} | ||
| }, | ||
| { | ||
| "measure_dir_name": "FakeReport", | ||
| "arguments": {} | ||
| } | ||
| ] | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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": "FakeModelMeasure", | ||
| "arguments": {} | ||
| }, | ||
| { | ||
| "measure_dir_name": "UnloadableMeasure", | ||
| "arguments": {} | ||
|
Comment on lines
+11
to
+12
There was a problem hiding this comment. This is an existing directory (findMeasure works), but it doesn't have a measure.xml |
||
| }, | ||
| { | ||
| "measure_dir_name": "FakeReport", | ||
| "arguments": {} | ||
| } | ||
| ] | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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": "FakeModelMeasure", | ||
| "arguments": {} | ||
| } | ||
|
Comment on lines
+6
to
+13
|
||
| ] | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| <?xml version="1.0"?> | ||
| <measure> | ||
| <schema_version>3.1</schema_version> | ||
| <name>fake_model_measure</name> | ||
| <uid>677b8fd3-2627-4516-b090-f6e47dc99fea</uid> | ||
| <version_id>162465e5-b6f6-419f-ab5f-c2fc4bd549e0</version_id> | ||
| <version_modified>2024-11-07T11:55:10Z</version_modified> | ||
| <xml_checksum>82D8F881</xml_checksum> | ||
| <class_name>FakeModelMeasure</class_name> | ||
| <display_name>A dumb ModelMeasure</display_name> | ||
| <description>Does nothing</description> | ||
| <modeler_description>Just for testing</modeler_description> | ||
| <arguments /> | ||
| <outputs /> | ||
| <provenances /> | ||
| <tags> | ||
| <tag>Envelope.Form</tag> | ||
| </tags> | ||
| <attributes> | ||
| <attribute> | ||
| <name>Measure Type</name> | ||
| <value>ModelMeasure</value> | ||
| <datatype>string</datatype> | ||
| </attribute> | ||
| <attribute> | ||
| <name>Measure Language</name> | ||
| <value>Ruby</value> | ||
| <datatype>string</datatype> | ||
| </attribute> | ||
| </attributes> | ||
| <files> | ||
| <file> | ||
| <version> | ||
| <software_program>OpenStudio</software_program> | ||
| <identifier>3.9.0</identifier> | ||
| <min_compatible>3.9.0</min_compatible> | ||
| </version> | ||
| <filename>measure.rb</filename> | ||
| <filetype>rb</filetype> | ||
| <usage_type>script</usage_type> | ||
| <checksum>DDA977B0</checksum> | ||
| </file> | ||
| </files> | ||
| </measure> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| This doesnt have a xml |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
| 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); | ||
| } | ||
|
Comment on lines
+71
to
+73
There was a problem hiding this comment. 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 I think that's pretty broken, but it's outside of the scope of this PR to fix it. There was a problem hiding this comment. As long as we document this to fix later that's good There was a problem hiding this comment. I know it might seem a bit backwards, but perhaps the There was a problem hiding this comment. 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. There was a problem hiding this comment. Meh. You can guess why it's the way it is.
People using ruby especially would try to do |
||
| } | ||
|
|
||
| WorkflowJSON_Impl::WorkflowJSON_Impl(const openstudio::path& p) { | ||
|
|
@@ -851,6 +857,65 @@ namespace detail { | |
| } | ||
| } | ||
|
|
||
| bool WorkflowJSON_Impl::validateMeasures() const { | ||
| // TODO: should we exit early, or return all problems found? | ||
|
Comment on lines
+860
to
+861
There was a problem hiding this comment. @DavidGoldwasser @kbenne thoughts here? I went with the "Find all problems" approach. There was a problem hiding this comment. 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 There was a problem hiding this comment. 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. There was a problem hiding this comment. The rationale of catching this early is perhaps sound, but it's also slow. Not sure what's better. |
||
|
|
||
| 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; | ||
| } | ||
|
Comment on lines
+863
to
+882
There was a problem hiding this comment. For each step, check we can find the measure directory, and it's a valid BCL Measure. |
||
|
|
||
| // 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; | ||
|
Comment on lines
+884
to
+914
There was a problem hiding this comment. And ensure the measures are in the right order |
||
| } | ||
|
|
||
| return result; | ||
| } | ||
| } // namespace detail | ||
|
|
||
| WorkflowJSON::WorkflowJSON() : m_impl(std::shared_ptr<detail::WorkflowJSON_Impl>(new detail::WorkflowJSON_Impl())) {} | ||
|
|
@@ -1123,6 +1188,10 @@ void WorkflowJSON::resetRunOptions() { | |
| getImpl<detail::WorkflowJSON_Impl>()->resetRunOptions(); | ||
| } | ||
|
|
||
| bool WorkflowJSON::validateMeasures() const { | ||
| return getImpl<detail::WorkflowJSON_Impl>()->validateMeasures(); | ||
| } | ||
|
|
||
| std::ostream& operator<<(std::ostream& os, const WorkflowJSON& workflowJSON) { | ||
| os << workflowJSON.string(); | ||
| return os; | ||
|
|
||
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.
Test 1