Skip to content
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

Wrap PythonPlugin:SearchPaths #214

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

joseph-robertson
Copy link
Contributor

@joseph-robertson joseph-robertson commented Dec 12, 2024

Pull request overview

Companion PR:

Link to relevant GitHub Issue(s) if appropriate:

Link to the Ubuntu 22.04 .deb installer to use for CI Testing. If not set, it will default to latest official release.
[OpenStudio Installer]:http://openstudio-ci-builds.s3-website-us-west-2.amazonaws.com/PR-5312/OpenStudio-3.9.1-alpha%2B8c127f63e3-Ubuntu-22.04-x86_64.deb

This Pull Request is concerning:

  • Case 1 - NewTest: a new test for a new model API class,
  • Case 2 - TestFix: a fix for an existing test. The GitHub issue should be referenced in the PR description
  • Case 3 - NewTestForExisting: a new test for an already-existing model API class
  • Case 4 - Other: Something else, like maintenance of the repo, or just committing test results with a new OpenStudio version.

Depending on your answer, please fill out the required section below, and delete the three others.
Leave the review checklist in place.


Case 1: New test for a new model API class

Please include which class(es) you are adding a test to specifically test for.
Include a link to the OpenStudio Pull Request in which you are adding the new classes, or the class itself if already on develop.

Work Checklist

The following has been checked to ensure compliance with the guidelines:

  • Tests pass either:

    • with official OpenStudio release (include version):

      • A matching OSM test has been added from the successful run of the Ruby one with the official OpenStudio release
      • The label AddedOSM has been added to this PR
      • All new out.osw have been committed
    • with current develop (incude SHA):

      • The label PendingOSM has been added to this PR
      • A matching OSM test has not yet been added because the official release is pending, but model_tests.rb has a TODO.
        def test_airterminal_cooledbeam_rb
          result = sim_test('airterminal_cooledbeam.rb')
        end
        
        # TODO: To be added in the next official release after: 2.5.0
        # def test_airterminal_fourpipebeam_osm
        #   result = sim_test('airterminal_fourpipebeam.osm')
        # end
      • No out.osw have been committed as they need to be run with an official OpenStudio version
  • Ruby test is stable: when run multiple times on the same machine, it produces the same total site kBTU.
    Please paste the heatmap png generated after running the following commands:

    • I ensured that I assign systems/loads/etc in a repeatable manner (eg: if I assign stuff to thermalZones, I do model.getThermalZones.sort_by{|z| z.name.to_s}.each do ... so I am sure I put the same ZoneHVAC systems to the same zones regardless of their order)
    • I tested stability using process_results.py (see python process_results.py --help for usage).
      Please paste the text output or heatmap png generated after running the following commands:
      # Clean up all custom-tagged OSWs
      python process_results.py test-stability clean
      # Run your test 5 times in a row. Replace `testname_rb` (eg `airterminal_fourpipebeam_rb`)
      python process_results.py test-stability run -n testname_rb
      # Check that they all passed
      python process_results.py test-status --tagged
      # Check site kBTU differences
      python process_results.py heatmap --tagged
      
  • Object has been added to autosize_hvac.rb to ensure the autosizedXXX values methods do work


Review Checklist

  • Code style (indentation, variable names, strip trailing spaces)
  • Functional code review (it has to work!)
  • Matching OSM test has been added or # TODO added to model_tests.rb
  • Appropriate out.osw have been committed
  • Test is stable
  • Object is tested in autosize_hvac as appropriate
  • The appropriate labels have been added to this PR:
    • One of: NewTest, TestFix, NewTestForExisting, Other
    • If NewTest: add PendingOSM or AddedOSM

@joseph-robertson joseph-robertson added NewTest PR type: a new test for a new model API class PendingOSM A matching OSM test has yet to be added with the next official OpenStudio Release labels Dec 12, 2024
@joseph-robertson joseph-robertson self-assigned this Dec 12, 2024
@joseph-robertson joseph-robertson changed the title Add dummy py script with import from plugin script. Wrap PythonPlugin:SearchPaths Dec 12, 2024
Copy link
Contributor

@jmarrec jmarrec Dec 18, 2024

Choose a reason for hiding this comment

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

This is used in python_plugin.osm. Did you delete it by mistake?

Comment on lines +126 to +131
# create the python plugin search paths object (this test should fail without it)
python_plugin_search_paths = model.getPythonPluginSearchPaths
python_plugin_search_paths.setAddCurrentWorkingDirectorytoSearchPath(true)
python_plugin_search_paths.setAddInputFileDirectorytoSearchPath(true)
python_plugin_search_paths.setAddepinEnvironmentVariabletoSearchPath(true)
python_plugin_search_paths.addSearchPath(File.dirname(__FILE__))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you confirm this fails without this object please?

@@ -0,0 +1 @@
print('hello world')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
print('hello world')
# This file is used by python_plugin_search_paths
print('hello world')

@jmarrec jmarrec marked this pull request as ready for review December 18, 2024 17:41
python_plugin_search_paths.setAddCurrentWorkingDirectorytoSearchPath(True)
python_plugin_search_paths.setAddInputFileDirectorytoSearchPath(True)
python_plugin_search_paths.setAddepinEnvironmentVariabletoSearchPath(True)
python_plugin_search_paths.addSearchPath(str(Path(__file__).parent))
Copy link
Contributor

Choose a reason for hiding this comment

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

I added a python version. Really the fact that this takes only a string bothers me.

Comment on lines +711 to +716
when 'python_plugin.osm', 'python_plugin_search_paths'
# We need to manually copy the supporting schedule into
# the testruns folder for the simulation to be able to find it
program_file_name = "#{File.basename(filename, File.extname(filename))}_program.py"
plugin_ori_path = File.join(File.dirname(__FILE__),
'model/simulationtests/python_plugin_program.py')
'model/simulationtests', program_file_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

When running the OSM version, copy the program.py file, which is computed as "#{stem}_program.py

python_plugin.osm -> python_plugin_program.py
python_plugin_search_paths.osm -> python_plugin_search_paths_program.py

Comment on lines +114 to +115
stem = File.basename(__FILE__, File.extname(__FILE__))
pluginPath = File.join(Dir.tmpdir, "#{stem}_program.py")
Copy link
Contributor

Choose a reason for hiding this comment

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

dynamic #{stem_program.py}

@jmarrec
Copy link
Contributor

jmarrec commented Dec 19, 2024

I need to fix the high-level tests that is failing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NewTest PR type: a new test for a new model API class PendingOSM A matching OSM test has yet to be added with the next official OpenStudio Release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants