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.

Sign up for GitHub

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

Jump to bottom

Add support for prefetching to sftp transport #849

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Add support for prefetching to sftp transport #849

wants to merge 1 commit into from
+27 −2

Conversation

davidszotten
Copy link

@davidszotten davidszotten commented Feb 23, 2025 •
edited
Loading

Title

Add support for prefetching to sftp transport

Motivation

Add optional prefetch_kwargs to ssh transport_params

These may be used to trigger prefetching, which can drastically improve throughput if downloading an entire file over sftp

Checklist

Before you create the PR, please make sure you have:

  • Picked a concise, informative and complete title
  • Clearly explained the motivation behind the PR
  • Linked to any existing issues that your PR will be solving
  • Included tests for any new functionality
  • Checked that all unit tests pass

@davidszotten davidszotten mentioned this pull request Feb 23, 2025
Open
@davidszotten
Add optional prefetch_kwargs to ssh transport
afe7da9 
Add optional `prefetch_kwargs` to ssh transport_params

These may be used to trigger prefetching, which can drastically improve
throughput if downloading an entire file over sftp
mpenkov
mpenkov reviewed Feb 26, 2025
View reviewed changes
smart_open/ssh.py
@@ -245,7 +248,9 @@ def open(path, mode='r', host=None, user=None, password=None, port=None, transpo
port: int, optional
The port to connect to.
transport_params: dict, optional
Any additional settings to be passed to paramiko.SSHClient.connect
Currently supported parameters:
connect_kwags: Any additional settings to be passed to paramiko.SSHClient.connect
Copy link
Collaborator

@mpenkov mpenkov Feb 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Typo here: connect_kwags

@ddelange
Copy link
Contributor

nit: can you move Fixes #848 out of the codeblock in the PR description? so it actually links, and auto-closes the issue when the PR merges

@piskvorky
Copy link
Owner

nit: can you move Fixes #848 out of the codeblock in the PR description? so it actually links, and auto-closes the issue when the PR merges

I've noticed this in several PRs, across several different authors. Not just here. Is this an issue with some PR template? Where does this "codeblock style" come from?

@ddelange ddelange mentioned this pull request Feb 26, 2025
Open
5 tasks
@davidszotten
Copy link
Author

nit: can you move Fixes #848 out of the codeblock in the PR description? so it actually links, and auto-closes the issue when the PR merges

I've noticed this in several PRs, across several different authors. Not just here. Is this an issue with some PR template? Where does this "codeblock style" come from?

yes i did think it was a bit odd but decided to follow the template. i see you're updating that too now 👍 will fix the description here

@davidszotten
Copy link
Author

is force-pushing ok for fixing these typos or do you still prefer new commits?

ddelange
ddelange reviewed Feb 26, 2025
View reviewed changes
smart_open/ssh.py
Comment on lines +252 to +253
connect_kwags: Any additional settings to be passed to paramiko.SSHClient.connect
prefetch_kwargs: Any additional settings to be passed to paramiko.SFTPFile.prefetch. The presence of this dict (even if empty) triggers prefetching.
Copy link
Contributor

@ddelange ddelange Feb 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
connect_kwags: Any additional settings to be passed to paramiko.SSHClient.connect
prefetch_kwargs: Any additional settings to be passed to paramiko.SFTPFile.prefetch. The presence of this dict (even if empty) triggers prefetching.
connect_kwargs: dict, optional
Any additional settings to be passed to paramiko.SSHClient.connect.
prefetch_kwargs: dict, optional
Any additional settings to be passed to paramiko.SFTPFile.prefetch. The presence of this dict (even if empty) triggers prefetching.

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

@mpenkov mpenkov mpenkov left review comments

@ddelange ddelange ddelange left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

use prefetch for sftp?
4 participants
@davidszotten @ddelange @piskvorky @mpenkov