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

v0.2.2 breaks SMBFilesystem #217

Closed
Tracked by #223
tharwan opened this issue May 14, 2024 · 4 comments · Fixed by #219
Closed
Tracked by #223

v0.2.2 breaks SMBFilesystem #217

tharwan opened this issue May 14, 2024 · 4 comments · Fixed by #219
Labels
bug 🐛 Something isn't working

Comments

@tharwan
Copy link
Contributor

tharwan commented May 14, 2024

hi, we are using upath to access an smb share. Unfortunately v0.2.2 broke the functionality.

It appears that the url components are somehow not passed on correctly.

E.g. UPath("smb://server.com/share/folder/file.csv")

will result in

ValueError: Failed to connect to 'server.comshare:445': [Errno 8] nodename nor servname provided, or not known

Edit: I would be happy and try to contribute a basic SMB implementation, is there any guide or example I can follow?

@tharwan
Copy link
Contributor Author

tharwan commented May 21, 2024

I appear I can fix this behaviour by implementing a very simple subclass that returns an absolute path:

class SMBPath(UPath):

    @property
    def path(self):
        return "/" + super().path

The docs from ffspec mention that:

Note: if using this with the open or open_files, with full URLs, there is no way to tell if a path is relative, so all paths are assumed to be absolute.

Which sounds like interpreting every path as absolute should be the default behaviour. Should I maybe file a bug with fsspec instead?

@ap--
Copy link
Collaborator

ap-- commented May 21, 2024

The fact that it worked with a previous version of UPath suggests that it's a UPath bug which got introduced during the refactor. Ideally we would add a SMBPath implementation to UPath to be able to give more guarantees for all the path methods.

I'm on holiday until the end of May and will then try to find some time to work on setting this up. We would need to implement an SMBPath subclass and the test fixtures to run the base test suite against it.

@ap--
Copy link
Collaborator

ap-- commented May 21, 2024

Edit: I would be happy and try to contribute a basic SMB implementation, is there any guide or example I can follow?

Sorry, just saw your comment. Thank you for offering to contribute! You would have to create tests in upath/tests/implementations/test_smb.py which requires you to write a fixture that sets up the smb filesystem via a docker container. There is code in the fsspec/filesystem_spec repository that does this. You would then put your minimal implementation in upath/implementations/smb.py and add an entry pointing at your new class into upath/registry.py

This should allow you to run the test suite against your class and make adjustments for various edge cases.

Cheers,
Andreas 😊

@ap-- ap-- added the bug 🐛 Something isn't working label May 21, 2024
@mraspaud
Copy link

mraspaud commented Jun 5, 2024

Hi, we have the same problem with an sftp-based filesystem:

path = UPath("ssh://myserver.mydomain.com/home/user1/Documents/hello.txt")
print(path.path)

'home/user1/Documents/hello.txt'

ie the first / is missing.

@ap-- ap-- closed this as completed in #219 Jun 14, 2024
@ap-- ap-- mentioned this issue Jun 14, 2024
16 tasks
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 a pull request may close this issue.

3 participants