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

MAINT: Do not install matplotlib backend without DISPLAY #132

Merged
merged 2 commits into from
May 31, 2018
Merged

MAINT: Do not install matplotlib backend without DISPLAY #132

merged 2 commits into from
May 31, 2018

Conversation

teddyrendahl
Copy link
Contributor

Description

Check that we have a DISPLAY environment variable before configuring the matplotlib backend for IPython. This destroys the entire session which is strange. We should allow users without displays to access non-graphics functions. A warning is printed to the terminal that alerts the user they don't have a configured DISPLAY so they will be aware something is strange.

Just for fun, I tried seeing what would happen if I ran a bluesky scan with BestEffortCallback to see if it exploded without matplotlib. This seems to error nicely and print an error that is consistent with our warning.

RuntimeError: Invalid DISPLAY variable

We could be extra nice and not configure the BEC for non display users but this seems extreme to me.

Motivation and Context

As a fix for pcdshub/Bug-Reports-and-Requests#6

How Has This Been Tested?

Tested locally without X-Forwarding. Still confused how this function works in Travis

Where Has This Been Documented?

@codecov-io
Copy link

codecov-io commented May 31, 2018

Codecov Report

Merging #132 into master will decrease coverage by 0.24%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #132      +/-   ##
==========================================
- Coverage   97.18%   96.93%   -0.25%     
==========================================
  Files          15       15              
  Lines         781      783       +2     
==========================================
  Hits          759      759              
- Misses         22       24       +2
Impacted Files Coverage Δ
hutch_python/cli.py 97.43% <50%> (-2.57%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c0c897...0a0d3a7. Read the comment docs.

Copy link
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

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

I'm still confused over this bug existing. I'm happy with this solution. I don't think we should go through and make sure all the graphics options are disabled, but we may consider going back to revising the default RE callbacks if people are ssh-ing around without x-forwarding enabled and want to collect data... Seems unlikely.

@teddyrendahl
Copy link
Contributor Author

teddyrendahl commented May 31, 2018

Looks like test just calls this function and checks for OSError. Where does that get raised? Presumably before we actually go to embed the shell / it affects the non-existent display

@ZLLentz
Copy link
Member

ZLLentz commented May 31, 2018

I would have expected travis to fail on

ipy_text = check_output([tstpython], universal_newlines=True,
input='unique_device\n')
with a CalledProcessError

@teddyrendahl teddyrendahl merged commit 5e3aa36 into pcdshub:master May 31, 2018
@teddyrendahl teddyrendahl deleted the no-display-no-prob branch May 31, 2018 20:49
@ZLLentz
Copy link
Member

ZLLentz commented May 31, 2018

There are two bizarre things with the test that didn't fail:

  • subprocess.Popen and affiliates behave subtly different than running scripts in a shell or with os.system
  • ipython shells that get stdin passed into them before entering the shell never reach the xqcb error and exit early

I looked into adding a test that fails before this PR and succeeds after, but the test is awkward and seems to subtly ruin the session after running. Hopefully we won't hit similar issues...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants