-
Notifications
You must be signed in to change notification settings - Fork 26
Resolve relative path relative to osw file #1965
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
base: master
Are you sure you want to change the base?
Conversation
| hpxml_path = args[:hpxml_path] | ||
| unless (Pathname.new hpxml_path).absolute? | ||
| hpxml_path = File.expand_path(hpxml_path) | ||
| hpxml_path = File.join(runner.workflow.absoluteRootDir.to_s, hpxml_path) |
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.
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.
Thanks, @rajeee. I'm glad to see the CI ran successfully but this makes me a bit nervous. For example, I know that PAT users had issues in the past. We'll need to check that it still works for that use case.
Is this meant to fix #1956?
| # @param args [Hash] Map of :argument_name => value | ||
| # @return [nil] | ||
| def set_file_paths(args) | ||
| def set_file_paths(runner, args) |
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.
Add to method documentation above
| "hpxml_path": "sample_files/base.xml", | ||
| "output_dir": "run", |
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.
I completely agree that the hpxml_path makes much more sense like this.
Can output_dir be anything or does it have to be "run"?
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.
It has to match the "run_directory" argument above (in line 2). If run_directory is not specified, it defaults to "run" and output_dir has to be "run" in that case. I think we should just remove this argument from the measure. We can find out the run_directory using runner.workflow.absoluteRunDir.to_s
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.
think we should just remove this argument from the measure.
We can't remove it, it is one of the most commonly used arguments (e.g. run_simulation.rb -x foo.xml -o my_output_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.
We can still allow run_simulation.rb to have output dir. Looks like the current behavior is that it creates a my_output_dir/run folder and places the results there. We can replicate this without needing the output_dir parameter for the measure.
Co-authored-by: Scott Horowitz <[email protected]>
It would be helpful to get @joseph-robertson feedback on whether it causes any problem for PAT.
Maybe - if we can get it all sorted out! |
Pull Request Description
Currently, relative paths in the measure arguments are resolved relative to the current working directory. This is problematic because:
When the measures are run directly from the workflow file (using
openstudio run -w workflow_file.osw), the current directory is something like './run/000_measure_class_name/'. So, in order to put the results into the run folder, one has to use..or absolute path. To refer to files at osw level, one has to use../../. This is confusing and not intuitive.When the measure is run through apply_measure, which is what the run_simulation.rb in OS-HPXML, and many meta measures and tests do, the current directory resolves to where the test/script is invoked from. This causes the relative path to resolve to different paths depending upon how the measure is called (via openstudio command or via the apply_measure). This makes it impossible to write osw files that can be run from both the command line using openstudio command or run through apply_measure by a test unless all paths are absolute.
This PR resolves the relative paths in the OSW relative to the OSW file itself, which alleviates this problem.
Checklist
Not all may apply:
EPvalidator.xml) has been updatedopenstudio tasks.rb update_hpxmls)HPXMLtoOpenStudio/tests/test*.rband/orworkflow/tests/test*.rb)openstudio tasks.rb update_measureshas been run