Skip to content
This repository was archived by the owner on Sep 29, 2025. It is now read-only.

Conversation

@xenoscopic
Copy link
Contributor

@xenoscopic xenoscopic commented Sep 15, 2025

This PR implements an auto-parallelizing transport to support model pulls. It doesn't wire anything up within this repository - that will happen downstream. This PR also refactors the existing resumable transport and its tests so that code is shared across the resumable and parallel implementations.

The parallel downloader currently uses temporary files on disk to support chunks in a way that's transparent to the http.RoundTripper interface (see transport/internal/bufferfile/fifo.go).

Most of this PR was written by AI, but it has been human-supervised, human-reviewed, and human-tested.

@xenoscopic xenoscopic marked this pull request as ready for review September 15, 2025 23:20
@ericcurtin ericcurtin requested a review from Copilot September 16, 2025 07:48
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements a parallel HTTP transport for improved model pull performance, alongside enhancements to the existing resumable transport. Key changes include:

  • New parallel transport implementation that automatically splits large downloads into concurrent byte-range requests
  • Refactored resumable transport with improved error handling and code organization
  • Shared common utilities extracted to reduce code duplication
  • Comprehensive test coverage using a shared testing framework

Reviewed Changes

Copilot reviewed 18 out of 19 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
transport/parallel/transport.go Core parallel transport implementation with concurrent byte-range downloading
transport/resumable/transport.go Refactored resumable transport with improved error handling and shared utilities
transport/internal/common/http_utils.go Shared HTTP utilities for range parsing and header handling
transport/internal/testing/fake_transport.go Common test framework for transport testing
transport/internal/bufferfile/fifo.go FIFO buffer implementation for temporary file-based streaming
tools/benchmarks/parallelget/main.go Command-line benchmark tool for comparing sequential vs parallel downloads

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@xenoscopic
Copy link
Contributor Author

Performance comparison on Docker Offload:

# ./pglinux 'https://production.cloudflare.docker.com/registry-v2/docker/registry/v2/blobs/sha256/57/57d4442d2a611cf9d54d118361ce29c3ae4db4a36d9e82328b706f6cb3604d2c/data
?<redacted>'
Benchmarking HTTP GET performance for: https://production.cloudflare.docker.com/registry-v2/docker/registry/v2/blobs/sha256/57/57d4442d2a611cf9d54d118361ce29c3ae4db4a36d9e82328b706f6cb3604d2c/data?<redacted>
Configuration: chunk-size=1048576 bytes, max-concurrent=4

Running non-parallel benchmark...
  Progress: [██████████████████████████████] 100.0% (4661213792/4661213792 bytes)
✓ Non-parallel: 4661213792 bytes in 2m26.335300079s (30.38 MB/s)
Running parallel benchmark...
  Progress: [██████████████████████████████] 100.0% (4661213792/4661213792 bytes)
✓ Parallel: 4661213792 bytes in 1m28.94439037s (49.98 MB/s)
Validating response consistency...
✓ Responses match perfectly

============================================================
PERFORMANCE COMPARISON
============================================================
🚀 Parallel was 1.65x faster than non-parallel
⏱️  Time saved: 57.390909709s (39.2%)

Detailed timing:
  Non-parallel: 2m26.335300079s
  Parallel:     1m28.94439037s
  Difference:   -57.390909709s

Performance comparison on Starlink:

$ ./parallelget 'https://production.cloudflare.docker.com/registry-v2/docker/registry/v2/blobs/sha256/83/8334b850b7bd46238c16b0c550df2138f0889bf433809008cc17a8b05761863e/data?<redacted>'
Benchmarking HTTP GET performance for: https://production.cloudflare.docker.com/registry-v2/docker/registry/v2/blobs/sha256/83/8334b850b7bd46238c16b0c550df2138f0889bf433809008cc17a8b05761863e/data?<redacted>
Configuration: chunk-size=1048576 bytes, max-concurrent=4

Running non-parallel benchmark...
  Progress: [██████████████████████████████] 100.0% (1915305312/1915305312 bytes)
✓ Non-parallel: 1915305312 bytes in 55.236737542s (33.07 MB/s)
Running parallel benchmark...
  Progress: [██████████████████████████████] 100.0% (1915305312/1915305312 bytes)
✓ Parallel: 1915305312 bytes in 54.093693708s (33.77 MB/s)
Validating response consistency...
✓ Responses match perfectly

============================================================
PERFORMANCE COMPARISON
============================================================
🚀 Parallel was 1.02x faster than non-parallel
⏱️  Time saved: 1.143043834s (2.1%)

Detailed timing:
  Non-parallel: 55.236737542s
  Parallel:     54.093693708s
  Difference:   -1.143043834s

Copy link
Contributor

@ilopezluna ilopezluna left a comment

Choose a reason for hiding this comment

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

I have tested the branch and verified that parallel transport works 👏👏.
I don’t have enough knowledge to give a thorough review, but I approve to unblock merging and to confirm that the parts I tested work as expected.

@xenoscopic
Copy link
Contributor Author

I'm going to merge this for now so that we can start pushing on the monorepo migration. This change won't bring in any new behavior - the parallel transport isn't wired up to anything by default. We will inherit some refinements and refactors to the existing resumable transport, but we have good test coverage for those.

@xenoscopic xenoscopic merged commit 6ef751e into main Sep 18, 2025
4 checks passed
@xenoscopic xenoscopic deleted the parallel-downloads branch September 18, 2025 20:39
doringeman pushed a commit to docker/model-runner that referenced this pull request Sep 23, 2025
…cker/model-distribution#130)

* transport: implement concurrent net/http.RoundTripper

Signed-off-by: Jacob Howard <[email protected]>

* transport: extract common code used across transports

Signed-off-by: Jacob Howard <[email protected]>

* transport: clean up semaphore implementation for unlimited case

Signed-off-by: Jacob Howard <[email protected]>

* transport: remove unnecessary CloneHeader function

Signed-off-by: Jacob Howard <[email protected]>

* transport: add better commenting to private field types

Signed-off-by: Jacob Howard <[email protected]>

* tools: add parallel download benchmark tool

Signed-off-by: Jacob Howard <[email protected]>

* tools: add progress output to parallelget benchmark

Signed-off-by: Jacob Howard <[email protected]>

* transport: refactor parallel transport to use FIFO implementation

Signed-off-by: Jacob Howard <[email protected]>

* transport: clean up FIFO tests and comments

Signed-off-by: Jacob Howard <[email protected]>

* tools: cleaned up parallelget implementation a bit

Signed-off-by: Jacob Howard <[email protected]>

* transport: refactored tests to use shared code

Signed-off-by: Jacob Howard <[email protected]>

* transport: improve code and comment styles

Signed-off-by: Jacob Howard <[email protected]>

* tools: clean up parallelget implementation and comments

Signed-off-by: Jacob Howard <[email protected]>

* transport: clean up tests and some formatting

Signed-off-by: Jacob Howard <[email protected]>

* transport: update code with GPT-5 recommendations

Signed-off-by: Jacob Howard <[email protected]>

* transport: implement resumable improvements from GPT-5

Signed-off-by: Jacob Howard <[email protected]>

* transport: consolidate FIFO constructors

Signed-off-by: Jacob Howard <[email protected]>

* transport: final fixups to AI-generated code

Signed-off-by: Jacob Howard <[email protected]>

* transport: avoid using net/http/httptest package

Signed-off-by: Jacob Howard <[email protected]>

* transport: clarify FIFO temporary file purpose

Signed-off-by: Jacob Howard <[email protected]>

* tools: don't show parallelget usage on error

Signed-off-by: Jacob Howard <[email protected]>

* transport: fix test in bufferfile

Signed-off-by: Jacob Howard <[email protected]>

* transport: remove time.Sleep instances in concurrency test

Signed-off-by: Jacob Howard <[email protected]>

---------

Signed-off-by: Jacob Howard <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants