Skip to content

Add parallel rsync sessions with fpsync - #202

Merged
ruaridhg merged 12 commits into
mainfrom
rmg/speed-up-rsync
Aug 3, 2026
Merged

Add parallel rsync sessions with fpsync#202
ruaridhg merged 12 commits into
mainfrom
rmg/speed-up-rsync

Conversation

@ruaridhg

@ruaridhg ruaridhg commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Towards #197

This PR:

  • rsync.py strips the trailing / to handle the user specifying the rsync paths with or without a trailing / and then adds a trailing / to rsync_source since it is required for the rsync command
  • rsync.py replaces simple rsync command with fpsync which runs 8 parallel rsync sessions

@ruaridhg ruaridhg changed the title Rmg/speed up rsync Add parallel rsync sessions with fpsync Jun 26, 2026
@ruaridhg
ruaridhg marked this pull request as ready for review June 26, 2026 14:32
@ruaridhg
ruaridhg requested a review from a team June 26, 2026 15:27
@ruaridhg ruaridhg closed this Jun 30, 2026
@ruaridhg ruaridhg reopened this Jul 2, 2026
@ruaridhg
ruaridhg removed the request for review from a team July 2, 2026 06:39
…ases URLs for XNAT downloads"

This reverts commit 26d1939.
@ruaridhg
ruaridhg requested a review from HChughtai July 27, 2026 16:01

@HChughtai HChughtai left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

fpsync looks useful, but a couple of comments. Have you seen improvements in practice using it?

Comment thread src/xmigrate/rsync.py Outdated
rsync_source,
"fpsync",
"-n",
"8",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why 8 parallel jobs? Will this depend on the number of physical and logical CPU cores, and so need to be something that is different dependning on the machine running it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah it will depend on physical and logical CPU cores so maybe having 8 as a default max and I could have it as an optional input i.e. parallel_jobs to the cli rsync command

Then within the cli command:

@app.command
def rsync(
    source: str,
    source_rsync: str,
    destination_rsync: str,
    log_dir: pathlib.Path = pathlib.Path("logs"),
    source_projects: list[str] | None = None,
    parallel_jobs: int | None = None,
) -> None:
    if parallel_jobs is None:
          parallel_jobs = min(8, os.cpu_count() or 1)
    
     run_rsync(
        destination_rsync,
        destination_proj,
        source_rsync,
        source_proj,
        parallel_jobs,
        )

What do you think @HChughtai ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, looks like a sensible approach. Only thing I would say is to add some handling so that you can't pass in numbers less than 1

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I'll do those changes and ask you to re-review after

Comment thread .github/workflows/tests.yml Outdated
@ruaridhg

Copy link
Copy Markdown
Contributor Author

fpsync looks useful, but a couple of comments. Have you seen improvements in practice using it?

Yep definitely improvements using it
7.7G (~7.5 minutes, 5,119 files) /data/xnat/archive/EPISURG (using fpsync ~90s so ~5x faster)
https://github.com/UCL-MIRSG/MIRSG/issues/354#issuecomment-4841335684

@ruaridhg
ruaridhg requested a review from HChughtai July 31, 2026 16:13
@ruaridhg
ruaridhg enabled auto-merge (squash) July 31, 2026 16:18
@ruaridhg
ruaridhg merged commit 766095d into main Aug 3, 2026
10 checks passed
@ruaridhg
ruaridhg deleted the rmg/speed-up-rsync branch August 3, 2026 11:44
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.

3 participants