-
Notifications
You must be signed in to change notification settings - Fork 49
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
Remove ds9 warnings when run under Python 3.6 #368
Remove ds9 warnings when run under Python 3.6 #368
Conversation
This is primarily to stop the ResourceWarning warnings that are created when running sherpa/image/tests/test_image.py under Python 3.6. In all-but-one case this is a simple fix, with the code being converted to use the context-manager approach to subprocess.Popen. These are the calls to xpaset and xpaget. The remaining issue is running ds9, which is meant to be run in the background: without explicitly using os.fork - which is not available on Windows - it's not clear how best to handle this so I have used a hack. It might be sensible to store the Popen instance when calling ds9 and storing it in the DS9Win class, but it's not clear if this helps this particular issue.
This replaces PR #364. It removes the decorator and |
Should we update the python-support button in |
would the commit I added do? |
LGTM - you can see the badge at https://github.com/DougBurke/sherpa/blob/remove-ds9-warnings-with-py36/README.md There's some other clean up needed in README.md, but I think it's more extensive than is warranted for this PR (e.g. there's mention of the latest release being 4.8.2), and should be done in a "Here's the documentation for the 4.9.1 release" PR). |
Unfortunately, this code does not work in Python 2, where Popen does not implement the context manager protocol. I am exploring some options, but maybe we should just revert this? The real problem is that we didn't pick up the issue from the CI jobs, so I am making sure we add a ds9 smoke test, and that we require ds9 in the CI jobs that install it as a dependency. |
I guess the
could be changed to
but, as indicated, I'm not sure what needs to be explicitly closed. Another option would be to add the ResourceWarning messages to |
Or there are some 2.7 solutions in https://stackoverflow.com/questions/30421003/exception-handling-when-using-pythons-subprocess-popen I think we could define |
With python 2.7 I get this:
as a quick check to see if it's still happening. |
For Python versions prior to 3.2 add in explicit code to make _Popen work as a context manager.
See PR #373 |
For Python versions prior to 3.2 add in explicit code to make _Popen work as a context manager.
Release Note
Update the DS9 code so that external processes are cleaned up properly, so to remove the potential
ResourceWarning
warnings when running DS9 on Python 3.6.Notes
Update the DS9 code so that external processes are cleaned up properly. This removes the
ResourceWarning
warnings raised when running thesherpa/image/tests/test_image.py
test. The Travis python 3.6 build is no longer allowed to fail.