Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Use symlinks instead of copies for test result_images.#10222
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Better:
with contextlib.suppress(FileNotFoundError): os.remove(expected_fname)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.
sure
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> . |
timhoffm commentedJun 11, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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. |
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.
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.
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.
Seems reasonable to me 👍
PR Summary
This seems to shave a few minutes(!) from the CI.
PR Checklist