Skip to content

Perf improvement for file copies #3135

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jan 16, 2020
Merged

Perf improvement for file copies #3135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0922344
Use speedcopy module for faster copies across network boundaries
rxxg Jan 8, 2020
811fc53
Use speedcopy module with bugfix
rxxg Jan 9, 2020
674541f
Use pyfastcopy for non-Windows systems using Python < 3.8
rxxg Jan 9, 2020
8fca139
Use released version of speedcopy library
rxxg Jan 15, 2020
e170852
Merge branch 'master' into master
rxxg Jan 15, 2020
083a630
Fallback to standard Python code in case of missing packages
rxxg Jan 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions dvc/system.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,31 @@
import errno
import logging
import os
import platform
import shutil

from dvc.compat import fspath
from dvc.exceptions import DvcException


logger = logging.getLogger(__name__)

if platform.system() == "Windows":
try:
import speedcopy

speedcopy.patch_copyfile()
except ImportError:
pass
else:
import sys

if sys.version_info < (3, 8):
try:
# Importing the module monkey-patches shutil.copyfile
import pyfastcopy # noqa: F401
except ImportError:
pass


class System(object):
@staticmethod
Expand Down Expand Up @@ -90,8 +107,6 @@ def _reflink_linux(src, dst):

@staticmethod
def reflink(source, link_name):
import platform

source, link_name = fspath(source), fspath(link_name)

system = platform.system()
Expand Down
2 changes: 2 additions & 0 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ def run(self):
"win-unicode-console>=0.5; sys_platform == 'win32'",
"pywin32>=225; sys_platform == 'win32'",
"networkx>=2.1,<2.4",
"speedcopy>=2.0.1",
"pyfastcopy>=1.0.3",
Comment on lines +78 to +79
Copy link
Contributor

@efiop efiop Jan 15, 2020

Choose a reason for hiding this comment

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

Both packages are not available in conda, so this PR will break our conda package 🙁Would you be willing to submit and maintain those packages in conda? Another simpler option is to make these packages optional, so that our System.copy can work with or without those packages. So pypi would be faster than conda package.

Copy link
Contributor

@efiop efiop Jan 15, 2020

Choose a reason for hiding this comment

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

It is not a dealbreaker, as we could help out with those packages too, we have quite some experience with conda-forge already. Just asking. 🙂

Copy link
Contributor

@Suor Suor Jan 16, 2020

Choose a reason for hiding this comment

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

Also, these deps should be conditional, i.e. none are needed in Python 3.8/Linux

Copy link
Contributor Author

@rxxg rxxg Jan 16, 2020

Choose a reason for hiding this comment

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

Hi @efiop, I won't have enough time to maintain conda packages as well, I will let you guys make the call as to whether that is worthwhile - not forgetting that this code will potentially be deleted under Python 3.8.

I have updated the patch to fall back to standard code if packages are missing.

Copy link
Contributor

@efiop efiop Jan 16, 2020

Choose a reason for hiding this comment

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

@rxxg No worries. Thanks for the PR! We'll try to create and maintain those conda packages ourselves.

"flatten-dict>=0.2.0",
"texttable>=0.5.2",
]
Expand Down