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.

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

Bug: 2 install_hooks bugs #548

Merged
merged 4 commits into from
Nov 11, 2024
Merged

Bug: 2 install_hooks bugs #548

merged 4 commits into from
Nov 11, 2024

Conversation

kbolashev
Copy link
Member

@kbolashev kbolashev commented Nov 10, 2024

Bug 1: Paths with ~ in them not getting expanded to the user home.

Example:

install_hooks(...)
with open("~/some/file.txt") as f:
    print(f.read())

The tilde wasn't being expanded and the wrong path was being read as a result (it was reading $CWD/~/some/file.txt)

Bug 2: Recursive call in DagsHubFilesystem.listdir() for passthrough paths:

If accessing a passthrough directory inside a mounted fs (e.g. something with site-packages), the listdir of the fs kept calling itself instead of passing through to the OS filesystem.
Example:

install_hooks(..., project_root=".")
print(os.listdir("./venv/site-packages"))

@kbolashev kbolashev added the bug Something isn't working label Nov 10, 2024
@kbolashev kbolashev self-assigned this Nov 10, 2024
Copy link

dagshub bot commented Nov 10, 2024

Comment on lines 330 to 335
return DagshubPath(self, None, None, Path(file))
if file == "":
return DagshubPath(self, None, None, orig_path)
abspath = Path(os.path.abspath(file))
return DagshubPath(self, None, None, Path(file))

expanded = os.path.expanduser(file)
orig_path = Path(expanded)
Copy link
Member

Choose a reason for hiding this comment

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

  1. You intentionally don't expand the orig_path before passing it into the DagshubPath? Just so I understand why. Maybe a comment explaining it?
  2. I think you introduced a bug here. The default open doesn't expanduser on the supplied path, from what I can gather. But now, with this change, every call to open will pass through us and get parsed here and then you use the returned absolute path in the call to __open. So I think instead, to be really safe, you should make sure to always call the underlying patched functions with exactly the same args as you were called with, making sure they're not modified in any way

Copy link
Member

Choose a reason for hiding this comment

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

I guess comment 2 is also relevant for __chdir and __listdir etc.

Copy link
Member Author

@kbolashev kbolashev Nov 10, 2024

Choose a reason for hiding this comment

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

[duplicating in-person chat]

You intentionally don't expand the orig_path before passing it into the DagshubPath? Just so I understand why. Maybe a comment explaining it?

Yes, this is because this original path is later reused in functions that have different behavior depending on the accessed paths (notable example: listdir, that returns different paths depending on whether passed an absolute or relative path)

Yea, you're right, I actually shouldn't be expanding it at all probably.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added explaining comment to each of the field of DagsHubPath and how it's being used

@kbolashev
Copy link
Member Author

Reverted the fix for "bug" 1, because after discussion we realised that it might just be a bug of the calling library that it's trying to access ~/... paths, since regular open also doesn't expand them

@kbolashev kbolashev merged commit ac6290e into master Nov 11, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants