Skip to content

remote: avoid chunking on webdav. Fixes #4796 #4828

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
skshetry merged 5 commits into from
Nov 11, 2020
Merged

remote: avoid chunking on webdav. Fixes #4796 #4828

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e091cc0
remote: avoid chunking on webdav. Fixes #4796
Nov 2, 2020
034b5dd
remote: avoid chunking on webdav. Fixes #4796
Nov 4, 2020
43e780c
remote: avoid chunking on webdav. Fixes #4796
Nov 6, 2020
09a99f3
remote: avoid chunking on webdav. Fixes #4796
Nov 6, 2020
34830ba
remote: avoid chunking on webdav. Fixes #4796
Nov 9, 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
35 changes: 17 additions & 18 deletions dvc/tree/webdav.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import threading
from collections import deque

from funcy import cached_property, wrap_prop
from funcy import cached_property, nullcontext, wrap_prop

from dvc.config import ConfigError
from dvc.exceptions import DvcException
Expand Down Expand Up @@ -216,31 +216,30 @@ def _download(self, from_info, to_file, name=None, no_progress_bar=False):
)

# Uploads file to remote
def _upload(self, from_file, to_info, name=None, no_progress_bar=False):
def _upload(
self, from_file, to_info, name=None, no_progress_bar=False,
):
# First try to create parent directories
self.makedirs(to_info.parent)

# Progress from HTTPTree
def chunks():
with open(from_file, "rb") as fd:
with Tqdm.wrapattr(
file_size = os.path.getsize(from_file)
with open(from_file, "rb") as fd:
progress_context = (
nullcontext(fd)
if file_size == 0
else Tqdm.wrapattr(
fd,
"read",
total=None
if no_progress_bar
else os.path.getsize(from_file),
total=None if no_progress_bar else file_size,
leave=False,
desc=to_info.url if name is None else name,
disable=no_progress_bar,
) as fd_wrapped:
while True:
chunk = fd_wrapped.read(self.CHUNK_SIZE)
if not chunk:
break
yield chunk

# Upload to WebDAV via buffer
self._client.upload_to(buff=chunks(), remote_path=to_info.path)
)
)
with progress_context as fd_wrapped:
self._client.upload_to(
Copy link
Collaborator

@skshetry skshetry Nov 6, 2020

Choose a reason for hiding this comment

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

@LucaButera, can you test with an empty file? If that works well, it should be good to merge.

Copy link
Contributor Author

@LucaButera LucaButera Nov 6, 2020

Choose a reason for hiding this comment

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

I think we have a problem with this. I get this error:

Traceback (most recent call last):
  File "/Users/lucabutera/dvc/dvc/cache/local.py", line 32, in wrapper
    func(from_info, to_info, *args, **kwargs)
  File "/Users/lucabutera/dvc/dvc/tree/base.py", line 356, in upload
    self._upload(  # noqa, pylint: disable=no-member
  File "/Users/lucabutera/dvc/dvc/tree/webdav.py", line 234, in _upload
    self._client.upload_to(
  File "/Users/lucabutera/dvc/.env/lib/python3.8/site-packages/webdav3/client.py", line 66, in _wrapper
    res = fn(self, *args, **kw)
  File "/Users/lucabutera/dvc/.env/lib/python3.8/site-packages/webdav3/client.py", line 438, in upload_to
    self.execute_request(action='upload', path=urn.quote(), data=buff)
  File "/Users/lucabutera/dvc/.env/lib/python3.8/site-packages/webdav3/client.py", line 208, in execute_request
    response = self.session.request(
  File "/Users/lucabutera/dvc/.env/lib/python3.8/site-packages/requests/sessions.py", line 530, in request
    resp = self.send(prep, **send_kwargs)
  File "/Users/lucabutera/dvc/.env/lib/python3.8/site-packages/requests/sessions.py", line 643, in send
    r = adapter.send(request, **kwargs)
  File "/Users/lucabutera/dvc/.env/lib/python3.8/site-packages/requests/adapters.py", line 469, in send
    for i in request.body:
TypeError: 'CallbackIOWrapper' object is not iterable

Which I think is related to the file being empty.
I created the file with touch filename.txt

Copy link
Collaborator

@skshetry skshetry Nov 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@LucaButera, you might be able to solve this with passing disable=True when the size is 0 (see the total kwarg is getting passed the total size).
If that does not work, we can use a context manager nullcontext in the case of 0 size, otherwise use Tqdm.wrappattr.

Copy link
Contributor Author

@LucaButera LucaButera Nov 9, 2020

Choose a reason for hiding this comment

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

@skshetry
So, I tried to set disable=no_progress_bar or os.path.getsize(from_file) == 0 to no success. It yields the same error as before.

Using nullcontext, instead, solves the issue. I implemented it in this fashion:

with open(from_file, "rb") as fd:
    progress_context = nullcontext(fd) if file_size == 0 else Tqdm.wrapattr(fd, "read", ...)
    with progress_context as fd_wrapped:
        self._client.upload_to(buff=fd_wrapped, remote_path=to_info.path)

but then another error occurs:

Traceback (most recent call last):
  File "/Users/lucabutera/dvc/dvc/cache/local.py", line 32, in wrapper
    func(from_info, to_info, *args, **kwargs)
  File "/Users/lucabutera/dvc/dvc/tree/base.py", line 356, in upload
    self._upload(  # noqa, pylint: disable=no-member
  File "/Users/lucabutera/dvc/dvc/tree/webdav.py", line 236, in _upload
    self._client.upload_to(
  File "/Users/lucabutera/dvc/.env/lib/python3.8/site-packages/webdav3/client.py", line 66, in _wrapper
    res = fn(self, *args, **kw)
  File "/Users/lucabutera/dvc/.env/lib/python3.8/site-packages/webdav3/client.py", line 438, in upload_to
    self.execute_request(action='upload', path=urn.quote(), data=buff)
  File "/Users/lucabutera/dvc/.env/lib/python3.8/site-packages/webdav3/client.py", line 226, in execute_request
    raise ResponseErrorCode(url=self.get_url(path), code=response.status_code, message=response.content)
webdav3.exceptions.ResponseErrorCode: Request to https://<user>@drive.switch.ch/remote.php/dav/files/<user>/datasets/d4/1d8cd98f00b204e9800998ecf8427e failed with code 411 and message: b'<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">\n<html><head>\n<title>411 Length Required</title>\n</head><body>\n<h1>Length Required</h1>\n<p>A request of the requested method PUT requires a valid Content-length.<br />\n</p>\n<hr>\n<address>Apache/2.4.18 (Ubuntu) Server at a01.drive.switch.ch Port 80</address>\n</body></html>\n'

Since I don't see the Content-lenght explicitly set anywhere I think it gets set somewhere at a lower level when the size is other than 0. Can this be the problem?

Regarding you last suggestion, to use Tqdm.wrapattr, I am not sure what you mean. I can try that if you elaborate further.

In the end, do we wish that an empty file gets uploaded anyway?

Copy link
Collaborator

@skshetry skshetry Nov 11, 2020

Choose a reason for hiding this comment

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

It'd be great, if we could support this. It seems if the file is empty, requests sends a chunked-encoding request anyway, which is what is causing the error. Let's ignore this for now, as it only affects you.

buff=fd_wrapped, remote_path=to_info.path
)

# Queries size of file at remote
def _file_size(self, path_info):
Expand Down