Use symlinks instead of copies for test result_images.#10222
Use symlinks instead of copies for test result_images.#10222dstansby merged 1 commit intomatplotlib:masterfrom
Conversation
|
power-cycled to get the gdb fix. |
|
OK, but are these tests really any faster on CI? 15 minutes 37 sec for the 3.6 test doesn't seem any faster than other recent tests... https://travis-ci.org/matplotlib/matplotlib/builds/325962079?utm_source=github_status&utm_medium=notification took 14 minutes 50 seconds (as a completely non-scientific comparison). |
|
Seems quite variable. Will probably try to accumulate some statistics on my side first... |
lib/matplotlib/testing/decorators.py
Outdated
| orig_expected_fname)) | ||
| raise ImageComparisonFailure(reason) | ||
| try: | ||
| try: # os.symlink errors if the target already exists. |
There was a problem hiding this comment.
Better:
with contextlib.suppress(FileNotFoundError):
os.remove(expected_fname)
|
What is the right way forward here? Should we try to verify the speed gain again? Should we drop this? Should we just accept on the basis that symlinking feels better and may even bring a performance gain (without test) and it's just slightly more complicated. |
|
Are there going to be issues on Windows filesystems?
…On Tue, Jun 11, 2019 at 4:27 PM Tim Hoffmann ***@***.***> wrote:
What is the right way forward here? Should we try to verify the speed gain
again? Should we drop this? Should we just accept on the basis that
symlinking feels better and may even bring a performance gain (without
test) and it's just slightly more complicated.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#10222?email_source=notifications&email_token=AACHF6CI34LIQ3XERB2ZDB3P2ADBTA5CNFSM4ELHZLJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXOMZ2A#issuecomment-501009640>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AACHF6DGTMK5LY555GUUDMTP2ADBTANCNFSM4ELHZLJA>
.
|
|
You need privileges to symlink on Windows, which a regular user on Windows does not have (https://docs.python.org/3/library/os.html?highlight=symlink#os.symlink). This is covered in the code with the The code is not compatible with WindowsXP and earlier, as that would throw a |
|
It feels better to use symlinks, although I have admittedly no numbers to show real improvements. I don't really care if this goes in or if we abandon it. |
timhoffm
left a comment
timhoffm
left a comment
There was a problem hiding this comment.
I've run the testsuite on master:
- 610s as is
- 565s with this patch
Using a PCI-E SSD, which should already be fast for file copying.
6 participants
PR Summary
This seems to shave a few minutes(!) from the CI.
PR Checklist