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

Add Py_SetProgramName to make sure sys.argv[0] is set #8631

Merged
merged 8 commits into from
Mar 22, 2021

Conversation

mitchute
Copy link
Collaborator

Pull request overview

  • Adds Py_SetProgramName to PluginManager to ensure sys.argv[0] is set

We have had issues while attempting to run E+ PythonPluings with third-party libraries (inside of a Docker container) with sys.argv[0] not being set. This should ensure this is set.

Pull Request Author

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

  • Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
  • Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
  • Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repair
  • Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process
  • If any diffs are expected, author must demonstrate they are justified using plots and descriptions
  • If changes fix a defect, the fix should be demonstrated in plots and descriptions
  • If any defect files are updated to a more recent version, upload new versions here or on DevSupport
  • If IDD requires transition, transition source, rules, ExpandObjects, and IDFs must be updated, and add IDDChange label
  • If structural output changes, add to output rules file and add OutputChange label
  • If adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependencies

Reviewer

This will not be exhaustively relevant to every PR.

  • Perform a Code Review on GitHub
  • If branch is behind develop, merge develop and build locally to check for side effects of the merge
  • If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
  • If feature, test running new feature, try creative ways to break it
  • CI status: all green or justified
  • Check that performance is not impacted (CI Linux results include performance check)
  • Run Unit Test(s) locally
  • Check any new function arguments for performance impacts
  • Verify IDF naming conventions and styles, memos and notes and defaults
  • If new idf included, locally check the err file and other outputs

@mitchute mitchute added the Defect Includes code to repair a defect in EnergyPlus label Mar 18, 2021
@mitchute mitchute added this to the EnergyPlus 9.5.0 milestone Mar 18, 2021
@mitchute mitchute self-assigned this Mar 18, 2021
@mitchute
Copy link
Collaborator Author

(@Myoldmopar feel free to correct any of the embedded Python vernacular or terminology)

This has bubbled up into a larger problem than we had originally anticipated. To summarize, some Python libraries rely on Py_SetProgramName to set sys.argv when running from an embedded Python library, as for example happens when running from the PythonPlugin system. The current version of E+ does not implement Py_SetProgramName; however, on my local machine (Mac), sys.argv was still be set. To verify, one can add the following to any PythonPlugin file and run EnergyPlus.

import sys
print(sys.argv)

On Mac, E+ has no problems running with or without the proposed change implemented in the commit here (b2a062e). I haven't tested this on Windows, but on Linux, the following is the result with or without adding Py_SetProgramName.

>>> AttributeError: module 'sys' has no attribute 'argv'

One workaround is to add the following at the top of each Python file:

import sys
if not hasattr(sys, 'argv'):
    sys.argv = ['']

But it looks like we're not the only ones who have had this problem. See here: https://bugs.python.org/issue32573 It looks like the issue is resolved by just moving up to Python 3.8.x. I've tested installing Python 3.8.5 on GitHub Actions, so I'm going to bump the versions of the packaged E+ versions up to that here in this branch. There should be no changes for developers related to this change, it only changes the Python version that is packaged up with E+ releases. Also, we may not actually need to add Py_SetProgramName, but the documentation does make it seem like a good idea to add it. See here: https://docs.python.org/3.8/c-api/init.html#c.Py_SetProgramName

@bonnema
Copy link
Contributor

bonnema commented Mar 18, 2021

@mitchute, on Windows:

C:\Users\ebonnema>python
Python 3.7.6 (tags/v3.7.6:43364a7ae0, Dec 19 2019, 00:42:30) [MSC v.1916 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> print(sys.argv)
['']
>>>

@mitchute
Copy link
Collaborator Author

@bonnema You'll have to add that to the top of any of the PythonPlugin files and then run it with EnergyPlus.

@bonnema
Copy link
Contributor

bonnema commented Mar 18, 2021

@bonnema You'll have to add that to the top of any of the PythonPlugin files and then run it with EnergyPlus.

🤦‍♂️

C:\Users\ebonnema\tmp>C:\EnergyPlusV9-4-0\energyplus.exe -w in.epw in.idf
EnergyPlus Starting
EnergyPlus, Version 9.4.0-998c4b761e, YMD=2021.03.18 15:42
[]
Adjusting Air System Sizing
Adjusting Standard 62.1 Ventilation Sizing
Initializing Simulation
Reporting Surfaces
Beginning Primary Simulation
Initializing New Environment Parameters
Warming up {1}
Warming up {2}
Warming up {3}
Warming up {4}
Warming up {5}
Warming up {6}
Starting Simulation at 07/01/2014 for ONE DAY
Writing tabular output file results using HTML format.
EnergyPlus Run Time=00hr 00min  1.45sec
EnergyPlus Completed Successfully.

C:\Users\ebonnema\tmp>

@mitchute
Copy link
Collaborator Author

@bonnema I guess we were just the lucky ones to find it on Linux.

@bonnema
Copy link
Contributor

bonnema commented Mar 18, 2021

@bonnema I guess we were just the lucky ones to find it on Linux.

... and why no one else complained 😄. I just added the Windows stuff for completeness on this PR. Your changes sound like a good fix all around.

@mitchute
Copy link
Collaborator Author

Right. I'd guess that the number of people who are using E+ on Linux with PyEMS is ..., well, @Myoldmopar, @jmarrec, and ... just us, I'd guess 🥇 🥈 🥉

@Myoldmopar
Copy link
Member

@mitchute any luck on this?

@mitchute
Copy link
Collaborator Author

@Myoldmopar yeah, this works as expected. I think updating to 3.8 would be the preferred solution, but GitHub isn’t behaving so this can be good enough this close to release.

@Myoldmopar
Copy link
Member

As expected, this has no effect on our CI setup, and I like these changes. I agree, going to a new Python version will make the best sense, and we can figure that out immediately after release. I like not making that change right now. Thanks for this fix!

@Myoldmopar Myoldmopar merged commit 5a12265 into develop Mar 22, 2021
@Myoldmopar Myoldmopar deleted the PyProgramName branch March 22, 2021 01:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Defect Includes code to repair a defect in EnergyPlus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants