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

test: testing sshfs with local ssh server #1013

Merged
merged 6 commits into from
Nov 2, 2023
Merged

test: testing sshfs with local ssh server #1013

merged 6 commits into from
Nov 2, 2023

Conversation

lobis
Copy link
Collaborator

@lobis lobis commented Oct 31, 2023

This PR adds an ssh server only to the ubuntu build of the CI to test accessing files over ssh via fsspec.

After testing it I realised that sshfs does not implement some interface required by uproot for this to work (cat_file) so it raises a NotImplementedError. I'm not sure if this is just not implemented and can be implemented or if there is a limitation why this has not been implemented (you cannot request a chunk of a file, you need to request it all?).

We could handle each fsspec case separately having a fallback in case some interface is not implemented, but I think it would complicate things too much.

After writing this PR I think it makes sense to raise a more detailed exception if fsspec does not implemented the required interface, so the user can post an issue in the corresponding fsspec repo. fsspec probably has a way to list the available methods (or it should!)

@lobis lobis marked this pull request as ready for review October 31, 2023 15:11
@lobis lobis requested a review from jpivarski October 31, 2023 15:11
Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

I hadn't known that fsspec provided an ssh backend, but it could be really useful. (I wonder about the usefulness of the S3 backend, given that S3 is a wrapper around HTTP.)

Does this test run in our CI? It's written in a way that it could run if sshd is installed, but that doesn't mean that it will run.

Okay, it doesn't actually run (Windows with Python 3.10):

SKIPPED [1] tests\test_0692_fsspec.py:87: ssh access to localhost failed with ssh access to localhost failed with ssh: connect to host localhost port 22: Connection refused

or in Ubuntu with Python 3.11:

SKIPPED [1] tests/test_0692_fsspec.py:98: sshfs needs to implement some interface

At least it is possible for this test to fail, if the attempt raised any exception other than NotImplementedError. (It would be strange for a test to not be able to fail.)

If you've tested this on your computer, you have sshd, and it passes, that's good enough for me. Even if you haven't, this is fine to merge as-is. If there's a problem with it that we discover later, then we'll deal with it then.

tests/test_0692_fsspec.py Outdated Show resolved Hide resolved
@lobis
Copy link
Collaborator Author

lobis commented Oct 31, 2023

If you've tested this on your computer, you have sshd, and it passes, that's good enough for me. Even if you haven't, this is fine to merge as-is. If there's a problem with it that we discover later, then we'll deal with it then.

No, I actually never tested this. After running the test I saw that sshfs does not implement cat_file interface (required to request a chunk of the file), so it's not possible to use uproot to read ssh files with fsspec at this time.

@jpivarski
Copy link
Member

So it will always fail with NotImplementedError until they (or we) add that feature. That's okay, it's a placeholder, and it will alert us to when they do add it (if it fails).

lobis and others added 3 commits October 31, 2023 12:05
@lobis lobis enabled auto-merge (squash) November 2, 2023 15:11
@lobis lobis merged commit 7d058d8 into main Nov 2, 2023
20 checks passed
@lobis lobis deleted the fsspec-ssh branch November 2, 2023 15:22
lobis added a commit that referenced this pull request Nov 15, 2023
* origin/main:
  test: testing sshfs with local ssh server (#1013)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants