Skip to content

Improvements for ssh file sources#21646

Open
bernt-matthias wants to merge 31 commits intogalaxyproject:devfrom
bernt-matthias:ssh-fs-improvements
Open

Improvements for ssh file sources#21646
bernt-matthias wants to merge 31 commits intogalaxyproject:devfrom
bernt-matthias:ssh-fs-improvements

Conversation

@bernt-matthias
Copy link
Contributor

@bernt-matthias bernt-matthias commented Jan 22, 2026

  • disable keepalive messages (why should we keep the connection alive)
    • while testing on my instance I had sometimes keepalive messages paramiko.transport DEBUG 2026-01-21 17:29:52,800 [pN:main.1,p:3872036,tN:Thread-8] Sending global request "keepalive@lag.net" every 10s for 2h. I could not reliably reproduce it, but I guess we do not need to keep connections alive, or?
  • make user mandatory: the default is the current user (which should be the Galaxy user)
  • allow to use them in templates

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@github-actions github-actions bot added this to the 26.1 milestone Jan 22, 2026
@jmchilton
Copy link
Member

Why are you making the user mandatory here? You mention the default in your PR description but you don't let it be set to None?

xref https://github.com/althonos/fs.sshfs/blob/master/fs/sshfs/sshfs.py#L36

@bernt-matthias
Copy link
Contributor Author

Can't get it working yet. When creating a file source from the template, the I get the error that the path is empty. Any idea @davelopez?

@bernt-matthias
Copy link
Contributor Author

bernt-matthias commented Feb 8, 2026

Why are you making the user mandatory here? You mention the default in your PR description but you don't let it be set to None?

xref https://github.com/althonos/fs.sshfs/blob/master/fs/sshfs/sshfs.py#L36

In the Galaxy context I found it not user friendly (and a bit "dangerous") to use the default implemented in sshfs: which is the account of the user running the Galaxy webserver. I can't see a scenario where this is useful for a Galaxy user - so I thought it's better to require users to set a username explicitly.

Edit: Guess my formulation was misleading: With "Galaxy user" I did not mean the user, but the user running the Galaxy server.

@davelopez
Copy link
Contributor

Now that #21704 is merged into dev, I suggest rebasing this and trying it out.
Remember that now we need to explicitly annotate optional fields with optional: true in the template.

It will also be cool if we could migrate this file source to fsspec as Nicola pointed out here #21823 (comment). But that can be a follow-up PR.

Let me know if I can help with something.

@bernt-matthias
Copy link
Contributor Author

Now that #21704 is merged into dev, I suggest rebasing this and trying it out. Remember that now we need to explicitly annotate optional fields with optional: true in the template.

Will do

It will also be cool if we could migrate this file source to fsspec as Nicola pointed out here #21823 (comment). But that can be a follow-up PR.

I'm certainly interested. I planned to learn more about file sources and their implementation in Galaxy (I have 1-2 that I would be interested to add). Are there any docs that could help me with the migration? Otherwise, maybe we can setup a meeting and you explain me a bit what is needed for the migration.

@davelopez
Copy link
Contributor

Are there any docs that could help me with the migration? Otherwise, maybe we can setup a meeting and you explain me a bit what is needed for the migration.

Sure! Let me know, and we can have a meeting to go through the examples or some of the details.

In general, the best documentation right now is the file sources already migrated to fsspec and the base class. In most cases, the migration or implementation is just a matter of defining well-typed configuration models and template configuration models and then doing the wiring with the base class, possibly overriding some of the base methods and functions as needed.

These are the relevant files:

Base Implementation

Concrete Implementations

@bernt-matthias
Copy link
Contributor Author

Added the fsspec implementation of the ssh filesource from here: https://github.com/bernt-matthias/galaxy/tree/sshfs-fsspec

I wont have time to work on tests ... If anyone wants to jump in? The unit tests worked locally for me (just need to fill the info in the yml file)

@davelopez davelopez marked this pull request as draft March 2, 2026 16:01
@davelopez davelopez marked this pull request as ready for review March 2, 2026 17:15
@davelopez davelopez force-pushed the ssh-fs-improvements branch from 3088ce2 to 33b4dda Compare March 2, 2026 17:20
@davelopez
Copy link
Contributor

I made some extra improvements, and Claude convinced me to write an in-memory ephemeral SFTP server to test instead of relying on local testing or Docker.
If that is too much, we can just drop the tests entirely since most remote file sources are not very test-friendly anyway.

@davelopez davelopez marked this pull request as draft March 3, 2026 12:20
bernt-matthias and others added 19 commits March 3, 2026 15:31
Moves `paramiko` imports and server implementation classes directly into the `sftp_server` pytest fixture. This change ensures that the `paramiko` library is only loaded when the fixture is actively used, making the test suite more modular and less prone to import errors if `paramiko` is not installed.

Adds a skip mechanism to the fixture, allowing tests that depend on the SFTP server to be automatically skipped if `paramiko` is unavailable, improving compatibility with diverse testing environments.
Prevents an `AttributeError` when `paramiko` attempts public-key authentication with a raw private key string instead of a `PKey` object. The code now explicitly sets the `pkey` parameter to `None` if the configured value is `None` or the string "None".

The example SSH file source template no longer includes the `pkey` option, preventing users from attempting to provide raw key content incorrectly.
Relocates the SSH file source tests from the unit test suite to a new Selenium integration test.

This change extracts the in-process SFTP server setup into a re-usable `SFTPServerMixin`. The new integration test then uses this mixin to verify the SSH file source functionality through the Galaxy UI, ensuring a more comprehensive end-to-end test.
@davelopez davelopez force-pushed the ssh-fs-improvements branch from 88b1d21 to 419aa7f Compare March 3, 2026 14:41
The SSH file source now correctly resolves and displays paths when configured with a `path` parameter pointing to a subdirectory on the remote server.

This change refines path translation logic within `_to_filesystem_path` and introduces `_adapt_entry_path` to correctly map remote filesystem paths back to virtual paths relative to the configured subdirectory.

A new selenium integration test validates the proper functionality of subdirectory-scoped SSH file sources.
@davelopez davelopez marked this pull request as ready for review March 3, 2026 17:35
@lldelisle
Copy link
Contributor

I look forward to have this implemented, this looks very interesting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants