Infra: allow for multiple install_hooks() running at the same time #448
Conversation
…interesting cases
| 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