Infra: allow for multiple install_hooks() running at the same time #448
Conversation
…interesting cases
| @@ -229,16 +253,22 @@ def get_file(self, path: str, revision: Optional[str] = None) -> bytes: | |||
| Returns: | |||
| bytes: The content of the file. | |||
| """ | |||
| res = self._http_request("GET", self.raw_api_url(path, revision)) | |||
| res = self._http_request("GET", self.raw_api_url(path, revision), timeout=None) | |||
There was a problem hiding this comment.
I don't understand the meaning of timeout=None and why it applies only here
There was a problem hiding this comment.
Getting a file has a high possibility of timing out when a user tries to download a huge file.
I set it to infinite wherever there is a chance that there'll be a big file transfer
| absolute_path: Optional[Path] | ||
| relative_path: Optional[Path] | ||
| original_path: Optional[Path] | ||
| def __init__(self, fs: "DagsHubFilesystem", file_path: Union[str, bytes, PathLike, "DagshubPath"]): |
There was a problem hiding this comment.
Why not reuse?
| def __init__(self, fs: "DagsHubFilesystem", file_path: Union[str, bytes, PathLike, "DagshubPath"]): | |
| def __init__(self, fs: "DagsHubFilesystem", file_path: PathTypeWithDagshubPath): |
There was a problem hiding this comment.
PathType/PathTypeWithDagshubPath allows an int, which I handle in all of the functions that create DagshubPath before creating it (and that usually means that it's a file descriptor and I can do nothing but just call the original function).
By not having an int here mypy complains that the int wasn't handled
| def parse_path(self, file_path: Union[str, bytes, PathLike, "DagshubPath"]) -> Tuple[Path, Optional[Path], Path]: | ||
| if isinstance(file_path, DagshubPath): | ||
| self.is_binary_path_requested = file_path.is_binary_path_requested | ||
| if file_path.fs != self.fs: |
There was a problem hiding this comment.
If the fs is the same, then we're just creating a copy of the input?
dagshub/streaming/dataclasses.py
Outdated
| return file_path.absolute_path, file_path.relative_path, file_path.original_path | ||
| if isinstance(file_path, bytes): | ||
| file_path = os.fsdecode(file_path) | ||
| orig_path = Path(file_path) |
There was a problem hiding this comment.
You don't want to save the original bytes representation of the file_path ?
There was a problem hiding this comment.
I want to have the og requested path as a Path object because it's more convenient to use.
Having the original path be both a Path object and an additional PathType would be confusing w.r.t. "which one should I be using", so I kept just the single thing I actually care about in it, which is whether the requested path was bytes or not
There was a problem hiding this comment.
I checked the usages, and it's probably alright to actually just carry over the original path, because I'm not working with it that much
There was a problem hiding this comment.
Changed it to carry over as-is
| if isinstance(file_path, bytes): | ||
| file_path = os.fsdecode(file_path) | ||
| orig_path = Path(file_path) | ||
| abspath = Path(os.path.abspath(file_path)) |
There was a problem hiding this comment.
This should always be relative to PWD ?
There was a problem hiding this comment.
Can you elaborate on the question? Didn't understand it
| abspath = Path(os.path.abspath(file_path)) | ||
| try: | ||
| relpath = abspath.relative_to(os.path.abspath(self.fs.project_root)) | ||
| if str(relpath).startswith("<"): |
There was a problem hiding this comment.
What does this condition mean?
There was a problem hiding this comment.
Honestly don't remember.
Maybe something relating to a different drive, but the docs say that relative_to will throw an error if they are on different drives altogether.
Probably an old leftover, so maybe should delete it
There was a problem hiding this comment.
This is a commit where it got added: 9174106
There was a problem hiding this comment.
If it's some IPython magic, then it's probably better to leave it, but could be useful to know how to trigger it at least 🤔
# Conflicts: # requirements-dev.txt
Infra changes:
DagsHubFilesysteminto two classes:DagsHubFilesystemthat does the file access stuff, andHookRouterthat runs the hook infra + routes the functions to the correct DHFilesystem.Functional changes:
install_hooks()can be running at the same time and any file access function will be routed to the correct filesystem.uninstall_hooks()now hasfsandpatharguments that remove specific filesystems. By default all filesystems will be removedget_mounted_filesystemsfunction that returns a list of tuples of(<full mount path>, <fs object>)Shouldn't be anything backwards-incompatible from a user API perspective