Skip to content

Conversation

@rajeee
Copy link
Collaborator

@rajeee rajeee commented Mar 24, 2025

Pull Request Description

Currently, relative paths in the measure arguments are resolved relative to the current working directory. This is problematic because:

  1. 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.

  2. 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:

  • Schematron validator (EPvalidator.xml) has been updated
  • Sample files have been added/updated (openstudio tasks.rb update_hpxmls)
  • Tests have been added/updated (e.g., HPXMLtoOpenStudio/tests/test*.rb and/or workflow/tests/test*.rb)
  • Documentation has been updated
  • Changelog has been updated
  • openstudio tasks.rb update_measures has been run
  • No unexpected changes to simulation results of sample files

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)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@shorowit shorowit left a 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)
Copy link
Contributor

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

Comment on lines +9 to +10
"hpxml_path": "sample_files/base.xml",
"output_dir": "run",
Copy link
Contributor

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"?

Copy link
Collaborator Author

@rajeee rajeee Mar 25, 2025

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

Copy link
Contributor

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).

Copy link
Collaborator Author

@rajeee rajeee Mar 25, 2025

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.

@rajeee
Copy link
Collaborator Author

rajeee commented Mar 25, 2025

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.

It would be helpful to get @joseph-robertson feedback on whether it causes any problem for PAT.

Is this meant to fix #1956?

Maybe - if we can get it all sorted out!

@shorowit shorowit marked this pull request as draft March 27, 2025 22:13
@shorowit shorowit moved this from Triage to In progress in OpenStudio-HPXML Apr 1, 2025
@shorowit shorowit moved this from In progress to Blocker/On Hold in OpenStudio-HPXML Apr 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Blocker/On Hold

Development

Successfully merging this pull request may close these issues.

3 participants