-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Delay the fc-list warning by 5 seconds #7532
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
Conversation
4f1ca3f
to
2a90d95
Compare
|
In that event, pass a |
Aw man, today is my day of implementing things and then finding out later they don't work in Python 2... But yes, I think we could implement this at least for Python 3 users? |
We optionally use the |
So in principle it's also possible to implement a solution based on writing a loop and using import time
start_time = time.time()
while time.time() - start_time < 5:
if p.poll():
output = pipe.communicate()[0]
break
else:
warnings.warn('Matplotlib is building the font cache using fc-list. This may take a moment.')
output = pipe.communicate()[0] But this doesn't seem as tidy. What do people think? |
Ick. |
There is a much simpler implementation I think using
|
@rmjarvis - I agree that looks nice, so it just depends whether people are happy to have an additional thread (which I think your solution requires behind the scenes). @tacaswell - do you have any opinions on this? |
Also, I think |
Then I'd argue strongly that the simple |
I agree with @dopplershift; using signals can get tricky and dangerous (and the sample code provided is incorrect because it does not reset the signal handler on exit), and applying that big hammer to this flea of a problem is not appropriate or acceptable for mpl. So it looks like timeout kwarg or nothing--and nothing is still a good option. Side note: python3 subprocess is using threads. |
Solving the double message would be nice, but I think it's only slow once, since after that it has cached results? IMO, though, it would be nice regardless to only see the warning if we're actually going to end up waiting, so regardless I'm 👍 on adding a 5 second pause before actually issuing a warning. |
On 2016/11/30 8:02 AM, Ryan May wrote:
Solving the double message would be nice, but I think it's only slow
once, since after that it's done.
What do you mean? The external call is made twice. It is much faster
the second time on the two systems I have tested, but I suspect this is
because the relevant disk sectors have been cached. (And on those
systems even the first call is fast.) The real question is whether, on
systems where the slowness of this call is a problem, the second call is
so fast that it doesn't matter.
|
Elaborating on @QuLogic's comment: yes, we use subprocess32 with python 2.7, but I think the problem is that although its communicate accepts a timeout argument, it is not supported in the way we need until python 3.3, when the argument was officially added: |
👍 to this solution 👍 to only fixing it for python3 if it is non-trivial to get it working in py2.7 👎 to signals 👎 to poll 👍 to re-writing the font finding to only need one call out, but that is a possibly bigger project. My main protest to threads is mpl having to maintain the thread related code. If upstream is providing an interface that hides the threads, I trust them 😈 . |
I've made it so it now only does this on Python 3. The solution to pass a kwarg dict to communicate doesn't work because for Python 2 we need to then emit the warning before the first communicate call. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it should work; not so easy to test.
try: | ||
output = pipe.communicate(timeout=5)[0] | ||
except subprocess.TimeoutExpired: | ||
warnings.warn('Matplotlib is building the font cache using ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: I would put this message in a local variable so that it doesn't need to be spelled out twice in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good - done!
The easiest way to test it locally is to replace the
|
output = pipe.communicate()[0] | ||
# We emit a warning if the process has not completed after 5 seconds. | ||
# We avoid showing it before since if this takes < 5 seconds, the user | ||
# probably won't notice. However, this only works on Python 3 because |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
subprocess32 doesn't have this? It is only an optional requirement, but that means that if people read our installation file (…), it'll work for 2.7
message = ('Matplotlib is building the font cache using fc-list. ' | ||
'This may take a moment.') | ||
|
||
if six.PY2: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't test for python2 but for whether subprocess has the attribute TimeoutExpired here:
hasattr(subprocess, TimeoutExpired)
subprocess32 being an optional dependency, that may work for python2.7.
Apart from my comment, this looks good. |
@efiring subprocess32 is based on the 3.2 module, but like many backports, it received continuous updates. The latest release (and presumably anything back to 3.3-equivalency) does include timeout, so as @NelleV says, we shouldn't skip this solely on the basis of running on Python 2.7. |
@@ -26,7 +26,7 @@ | |||
else: | |||
import subprocess | |||
|
|||
__all__ = ['Popen', 'PIPE', 'STDOUT', 'check_output', 'CalledProcessError'] | |||
__all__ = ['Popen', 'PIPE', 'STDOUT', 'check_output', 'CalledProcessError', 'TimeoutExpired'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be line-wrapped ❤️ pep8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yet more python2 compatibility issues.
Either this needs to be shimmed out more cautiously or subprocess32 needs to be a runtime dependancy.
@@ -36,6 +36,7 @@ | |||
STDOUT = subprocess.STDOUT | |||
CalledProcessError = subprocess.CalledProcessError | |||
check_output = subprocess.check_output | |||
TimeoutExpired = subprocess.TimeoutExpired |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not work on python2 unless subprocess32 is installed.
Python 2.7.12 (default, Nov 7 2016, 11:55:55)
[GCC 6.2.1 20160830] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import subprocess
>>> subprocess.TimeoutExpired
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: 'module' object has no attribute 'TimeoutExpired'
>>>
This looks like it is working on travis because subprocess32
in an install require (not a run-time require) so it is always getting installed on travis. I think that there is a way to install mpl such that this will fail.
We cant really make subprocess32 a full dependency, it's explicitly listed as not tested on windows see https://pypi.python.org/pypi/subprocess32/ |
Sorry for the delay, today is a feature freeze day for astropy so things are pretty busy there. I can get back to this next week, or if any of the maintainers want to push commits to this branch, feel free to. |
There's https://docs.python.org/2.7/library/threading.html#timer-objects (available both on Py2 and on Py3). Something like
should be good enough? (and you can cancel already expired timers) |
The Timer approach does seem cleaner - any objections to changing to that? |
That seems like no objections, but I'd squash out the other commits if we aren't going to use them. |
Changing implementation.
See #7596 for an alternative implementation. |
The original issue was solved by #7596. |
@anntzer - thanks for wrapping up the alternative implementation! |
In #5836 (comment), @tacaswell said:
If there is a non-thread based way to get a 5s delay I am 👍 on that, but bringing threads in this seems like complexity overkill.
The solution here doesn't use any extra threads/processes compared to what was being used before, and is a pretty simple/robust solution.
cc @tacaswell @efiring