-
Notifications
You must be signed in to change notification settings - Fork 222
For halted workflow, return before FT and OSM save #5539
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
Conversation
c55f9ad to
0b2eb37
Compare
🧪 Test Results DashboardSummary
|
| Run | XML File | Status |
|---|---|---|
| run1 | results.xml |
✅ Found |
| run3 | results.xml |
✅ Found |
| run2 | results.xml |
✅ Found |
| LOG(Info, "Workflow halted, skipping OSM to IDF translation"); | ||
| // This allows model arguments to still be calculated | ||
| workspace_ = Workspace{}; | ||
| return; |
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.
that's ok.
src/workflow/OSWorkflow.cpp
Outdated
| // Skip saving the OSM if halted | ||
| if (runner.halted()) { | ||
| LOG(Info, "Workflow halted, skipping saving the OSM"); | ||
| return; | ||
| } |
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.
Not sure whether we actually want this or not.
This is saving to root dir only if debug.
And the in.osm is saved in the run/ folder anyways.
Also, It's say the source of truth should be the classic CLI. Run this example https://github.com/NREL/OpenStudio-workflow-gem/blob/4a799dbf4c5477610c9e9665dfa7729fe74fc52e/spec/files/halt_workflow_osw/halt_workflow.osw
cd OpenStudio-workflow-gem/spec/files/halt_workflow_osw
openstudio classic --verbose run --show-stdout --debug -w halt_workflow.osw
I do end up with the "in.osm" (and in.idf, empty) in the root dir.
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.
git clean -fxd
openstudio classic --verbose run --show-stdout -w halt_workflow.osw
git ls-files . --ignored --exclude-standard --others
out.osw
run/data_point.zip
run/data_point_out.json
run/finished.job
run/in.osm
run/measure_attributes.json
run/objectives.json
run/pre-preprocess.idf
run/results.json
run/run.log
run/started.job
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.
Before fix
git clean -fxd
openstudio run --show-stdout -w halt_workflow.osw
git ls-files . --ignored --exclude-standard --others
out.osw
run/data_point.zip
run/data_point_out.json
run/finished.job
+ run/in.idf
run/in.osm
run/measure_attributes.json
run/pre-preprocess.idf # And this is full of stuff
run/results.json
run/run.log
run/started.jobThere 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.
With this PR:
git clean -fxd
$os_build/Products/openstudio run --show-stdout -w halt_workflow.osw
git ls-files . --ignored --exclude-standard --others
I still have run/in.idf and run/pre-processed.idf but at least they contain only the "Version" object.
(py312)(3.2.2)julien@halt_workflow_osw (master *=)$ cat run/in.idf
Version,
25.2.0; !- Version Identifier
(py312)(3.2.2)julien@halt_workflow_osw (master *=)$ cat run/pre-preprocess.idf
Version,
25.2.0; !- Version Identifier
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.
So with halt workflow (debug or not), we'll now expect no in.idf or pre-preprocess.idf, but we'll still expect an (almost empty) in.osm?
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.
Yes, in.osm would not be necessarily empty
Seed + whatever measure ran until halted.
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.
And the reason for preserving in.osm is mainly to stay consistent with the classic workflow? You don't think it makes more sense to not produce the in.osm?
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.
Consistent with workflow-gem.
And I could see myself wanting to look at in.osm (I could halt workflow before FT for that reason: don't fail, but produce the model with measures applied). Anyways, if I even think there's a valid application out there and I don't see a strong drawback, I'd want to keep the existing behavior in check.
And I do not see a strong drawback here (only one I can think of is disk space, but that's not a very pressing concern).
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.
Note that you can specify the "completed status" when you halt the workflow, and it can be "Success", "Fail", "Invalid", and "Cancel". We are using "Invalid" in ResStock, which arguably could mean that you shouldn't get an in.osm. But it's not the end of the world. The most important thing is to not get an in.idf, considering that you won't be running the simulation.
|
@joseph-robertson I'm tempted to say that the RunPreprocess check for halted is misplaced in the original workflow gem: https://github.com/NREL/OpenStudio-workflow-gem/blob/4a799dbf4c5477610c9e9665dfa7729fe74fc52e/lib/openstudio/workflow/jobs/run_preprocess.rb#L46-L49 the C++ implementation does it exactly the same. OpenStudio/src/workflow/RunPreProcess.cpp Lines 50 to 54 in 887ed96
I don't see why you would create pre-process.idf and apply energyplus output requests if you halted, and didn't create an IDF to begin with. (But then again, I completely fail to see what haltWorkflow is all about since I never had a use case for it) |
…): no point creating pre-process.idf nor collecting E+ output requests
It’s possible that ResStock (and I assume ComStock) are the only users of it. It’s used to skip simulations when an upgrade doesn’t apply to a given building sample. See #2601 |
|
@anchapin the windows node is offline and can't be launched from jenkins admin panel https://ci.openstudio.net/computer/openstudio%2Dwin%2Dserver%2Dvs%2D2019%2Dssh%2Dincr%2Daws/log |
I'm working on making a new windows node on AWS since our old one had to be shut down. I'm hoping it will be ready soon but I'm dealing with 'packer' difficulties. |
|
CI Results for bd3c759:
|


Pull request overview
File run_translation.rb in the OpenStudio-workflow-gem has a return before FT, but OSWorkflow.cpp in OpenStudio's src/workflow folder does not. We'll now expect no in.idf or pre-preprocess.idf.
Furthermore, the OpenStudio-workflow-gem saves the model to in.osm even if the workflow was halted. This PR maintains that behavior.
Pull Request Author
src/model/test)src/energyplus/Test)src/osversion/VersionTranslator.cpp)Labels:
IDDChangeAPIChangePull Request - Ready for CIso that CI builds your PRReview Checklist
This will not be exhaustively relevant to every PR.