-
Notifications
You must be signed in to change notification settings - Fork 571
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix HfFileSystemFile
when init fails + improve error message
#1805
Fix HfFileSystemFile
when init fails + improve error message
#1805
Conversation
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.
Nice thank you !
The documentation is not available anymore as the PR was closed or merged. |
Thanks for the quick approval 😉 Should be released soon btw :) |
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. |
@@ -413,7 +413,21 @@ class HfFileSystemFile(fsspec.spec.AbstractBufferedFile): | |||
def __init__(self, fs: HfFileSystem, path: str, revision: Optional[str] = None, **kwargs): | |||
super().__init__(fs, path, **kwargs) | |||
self.fs: HfFileSystem | |||
self.resolved_path = fs.resolve_path(path, revision=revision) | |||
|
|||
mode = kwargs.get("mode", "r") |
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.
fsspec
's AbstractBufferedFile
has "rb" as the default mode...
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.
Oh ok, thanks for letting me know. I'll open a follow-up PR to update that but in any case it doesn't change anything (this value is used only to check if it's "w"
or "wb"
)
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 made this commit (18d0ae2) to avoid confusion
Fix #1800 (?)
This PR does 2 things:
HfFileSystemFile.__init__
fails because repo/revision do not exist,self.resolved_path
is not set. As a consequence the magic__del__
(called just after since init failed) also fails because the file cannot be closed. This PR fixes this by not closing the file ifresolved_path
is not set.FileNotFoundError
error message is improved to let the user know why the file is not found (either repo not found, repo_id not valid or revision not found). Also mentions to the user that the repo must exist before writing a file to it. @lhoestq please let me know what you think of the updated message.I am not a big fan of auto-creating the repo/revision if not found. Biggest problem is that we would need to provide additional information (typically should it be created as
private
? and if it is a Space repo, which sdk to specify?). I think for now improving the error message is already a good enough solution.