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

Don't fail matplotlib related doctests if DISPLAY is unset #22441

Closed
infinity0 mannequin opened this issue Feb 25, 2017 · 16 comments
Closed

Don't fail matplotlib related doctests if DISPLAY is unset #22441

infinity0 mannequin opened this issue Feb 25, 2017 · 16 comments

Comments

@infinity0
Copy link
Mannequin

infinity0 mannequin commented Feb 25, 2017

Debian (and I'm sure other distros)'s autobuilders run with unset DISPLAY, and the tests fail without this patch.

Component: interfaces

Author: Ximin Luo

Branch/Commit: u/infinity0/don_t_fail_plot_py_doctest_if_display_is_unset @ a017372

Reviewer: François Bissey

Issue created by migration from https://trac.sagemath.org/ticket/22441

@infinity0 infinity0 mannequin added this to the sage-7.6 milestone Feb 25, 2017
@infinity0 infinity0 mannequin added the p: major / 3 label Feb 25, 2017
@infinity0
Copy link
Mannequin Author

infinity0 mannequin commented Feb 25, 2017

@infinity0
Copy link
Mannequin Author

infinity0 mannequin commented Feb 25, 2017

Commit: a017372

@infinity0

This comment has been minimized.

@infinity0
Copy link
Mannequin Author

infinity0 mannequin commented Feb 25, 2017

Author: Ximin Luo

@infinity0
Copy link
Mannequin Author

infinity0 mannequin commented Feb 25, 2017

New commits:

a017372Don't fail matplotlib related doctests if DISPLAY is unset

@infinity0 infinity0 mannequin added t: bug labels Feb 25, 2017
@infinity0 infinity0 mannequin changed the title Don't fail plot.py doctest if DISPLAY is unset Don't fail matplotlib related doctests if DISPLAY is unset Feb 25, 2017
@videlec
Copy link
Contributor

videlec commented Feb 25, 2017

comment:4

I found weird that this modification has to be done only in two places... there are plots everywhere in the doctests! Why is that? Wouldn't it be better to set it automatically by the doctest engine?

@infinity0
Copy link
Mannequin Author

infinity0 mannequin commented Feb 25, 2017

comment:5

Debian is on matplotlib v2 and Sage is still using v1.5 (see our status page).

I did initially also add it into other places, but only these two were needed in the end to fix the test failures. Perhaps it runs the other plots after running these tests, so the "good" settings have already been set by then?

Agreed that this is best done in the main code itself; probably not even in the doctest engine, but the Sage-matplotlib interface. I'm not certain where would be the best place though, I'm not familiar with the code.

@infinity0
Copy link
Mannequin Author

infinity0 mannequin commented Feb 25, 2017

comment:6

It might be cleaner to patch matplotlib itself, I've filed a ticket on their side too.

@jdemeyer
Copy link

comment:7

I cannot reproduce this problem. I'm sure that many bots run Sage with DISPLAY unset, so there must be something more going on...

@infinity0
Copy link
Mannequin Author

infinity0 mannequin commented Feb 25, 2017

comment:8

Looks like upstream rejected my suggestion. I can't reproduce this now on my main computer either, but I'll try it again in Debian's build-in-a-chroot program later.

env -i PATH=/usr/bin:/bin HOME=$HOME sage -c 'plot(x^2, (x,0,5))'

They did point me to the MPLBACKEND envvar so perhaps Sage's doctesting framework can set this envvar, assuming the "real cause" isn't something else.

@jdemeyer
Copy link

comment:9

Any further news? Can this be reproduced in vanilla Sage?

@fchapoton
Copy link
Contributor

comment:10

see #23696 for matplotlib update

@saraedum
Copy link
Member

comment:11

It appears that #23696 fixed this:

+# Make sure the agg backend is selected during doctesting
+# This needs to be done before any other matplotlib calls.
+import matplotlib
+matplotlib.use('agg')

So, can this be closed now?

@saraedum saraedum removed this from the sage-7.6 milestone Mar 24, 2018
@kiwifb
Copy link
Member

kiwifb commented Mar 24, 2018

comment:12

Yes, I didn't know there was a matching ticket to the debian patch. But this was definitely part of my target in #23696.

@kiwifb
Copy link
Member

kiwifb commented Mar 24, 2018

Reviewer: François Bissey

@videlec
Copy link
Contributor

videlec commented May 18, 2018

comment:13

closing positively reviewed duplicates

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

No branches or pull requests

5 participants