Skip to content
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

Jump to bottom

FileSystem: handle encoding via kwargs. #7694

Merged
merged 1 commit into from May 5, 2022
Merged

FileSystem: handle encoding via kwargs. #7694

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions dvc/fs/base.py
View file
Open in desktop
Expand Up @@ -179,10 +179,11 @@ def open(
self,
path: AnyFSPath,
mode: str = "r",
encoding: Optional[str] = None,
**kwargs,
) -> "IO": # pylint: disable=arguments-differ
return self.fs.open(path, mode=mode, encoding=encoding, **kwargs)
if "b" in mode:
kwargs.pop("encoding", None)
Comment on lines +184 to +185
Copy link
Member

Choose a reason for hiding this comment

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

Hm, why do we even need to pop it? In the #7691 we are not even getting the encoding here.

Copy link
Member

Choose a reason for hiding this comment

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

RepoFS and DvcFS always sends encoding=None to the filesystems even in binary mode. open() is not fsspec compliant and can be fixed at that time in other filesystems. For now, I think this is the simplest approach.

Copy link
Member

Choose a reason for hiding this comment

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

Let's fix repofs/dvcfs then if that is not too much work. Seems like it is a simple arg adjustment. Unless I'm missing something.

Copy link
Member

@skshetry skshetry May 5, 2022 •
edited

Choose a reason for hiding this comment

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

I'd still try to avoid this bug here even if we fix repofs/dvcfs, until we fix this on fsspec's side.

return self.fs.open(path, mode=mode, **kwargs)

def cat(
self,
Expand Down