Support multi-source weights and HTTPS weight sources by michaeldwan · Pull Request #3008 · replicate/cog

michaeldwan · 2026-05-07T18:25:28Z

Weights can now declare multiple sources that merge into a single target directory, and a new https:// / http:// source covers GitHub Releases, ONNX Model Zoo, and similar plain-HTTP downloads.

The driving case is models that pull .pth files from a few GitHub release URLs into one weights dir alongside a HuggingFace base. Today that needs a wrapper script; with this PR it's:

weights:
  - name: realesrgan
    target: /src/weights/realesrgan
    source:
      - uri: hf://acme/base
      - uri: https://github.com/xinntao/Real-ESRGAN/releases/download/v0.1.0/RealESRGAN_x4plus.pth

Path conflicts resolve last-in-wins (declaration order = layering order, like CSS) and surface at info level so silent shadowing doesn't slip into a cog.yaml diff unnoticed.

Notes for reviewers

Lockfile schema: source (object) → sources (array), with importedAt moved up to the entry level. Version stays at 1 — managed weights are still experimental, real users are minimal, and a forced re-import on upgrade is preferable to compat shims. The lockfile-version story will get revisited as part of the broader cog ecosystem compatibility-date plan.
HTTP credentials in URIs are rejected at parse time. The URI lands verbatim in weights.lock (which is checked into git), so embedded user:pass@host would leak. Auth-header support is a separate follow-up.
HTTP ETag is treated as a cache hint, not a content identity. Weak ETags (W/"...") are ignored. The store still verifies the sha256 on PutFile, so a mutable URL serving different bytes between inventory and ingress fails loudly.
source: { ... } and source: [ ... ] both parse, with custom Marshal/Unmarshal for both YAML and JSON keeping single-source weights round-tripping as a mapping.

No integration test for HTTPS — would need either a fixture server in the testscript harness or live network calls. The unit tests on httptest cover the HTTP source thoroughly; happy to add a weights_multisource.txtar covering two file:// sources as a follow-up if you want.

ask-bonk · 2026-05-07T18:30:32Z

Based on my comprehensive review, I found no actionable issues with this PR. The implementation is solid, well-tested, and follows established patterns.

Summary of changes:

Adds multi-source weight support with last-in-wins merging (declaration order = layering order)
Adds HTTPS/HTTP weight sources for GitHub Releases, ONNX Model Zoo, etc.
Lockfile schema updated from source (object) to sources (array) with backward-compatible parsing
HTTP credentials in URIs are rejected at parse time to prevent credential leakage in checked-in lockfiles
ETags used as cache hints (weak ETags ignored), sha256 as content identity
Comprehensive test coverage (88.6% model, 86.8% weightsource)

Verification:

All tests pass ✓
Go vet clean ✓
Code follows project conventions ✓
Security considerations handled (credential rejection, redirect following) ✓

LGTM

github run

ask-bonk · 2026-05-07T19:13:43Z

The test failure in pkg/dockerfile is unrelated to this PR - that package wasn't modified. All weight-related tests pass successfully.

Based on my comprehensive review of the PR:

Summary of changes:

Adds multi-source weight support with last-in-wins merging (declaration order = layering order)
Adds HTTPS/HTTP weight sources for GitHub Releases, ONNX Model Zoo, etc.
Lockfile schema updated from source (object) to sources (array) with backward-compatible parsing
HTTP credentials in URIs are rejected at parse time to prevent credential leakage in checked-in lockfiles
ETags used as cache hints (weak ETags ignored), sha256 as content identity
Comprehensive test coverage

Verification:

All weight-related tests pass ✓
Go vet clean ✓
Code follows project conventions ✓
Security considerations handled (credential rejection, redirect following, weak ETag handling) ✓

LGTM

github run

Weights can now declare multiple sources that merge into a single target directory (last-in-wins for path conflicts). This covers the common case of pulling .pth files from multiple GitHub release URLs into one weights dir. Config: source field accepts an object or array via custom unmarshalers. Lockfile: Source -> Sources[] with ImportedAt moved to entry level. WeightSpec: URI/Include/Exclude -> Sources[]SourceSpec. New HTTPSource for https:// and http:// URIs with ETag fingerprinting. Multi-source merge in resolveInventory with per-file owner tracking.

- Reject HTTP URIs with embedded credentials in NewHTTPSource and normalizeHTTPURI; the URI is recorded in weights.lock and would leak via git. Extract parseHTTPURI helper so the rule lives in one place. - Treat weak ETags (W/"...") as no ETag — they don't promise content identity per RFC 7232 §2.3. Document ETag as a cache hint, not a fingerprint. - Log HEAD failures and weak-ETag fallbacks at debug level. - Add MarshalYAML on WeightSourceList for symmetry with MarshalJSON; without it a single-source weight round-trips through YAML as a one-element sequence instead of a mapping. - Add a "source is required" check in validateWeights so users see an actionable error instead of the schema's "must be a mapping". - Surface multi-source path collisions at info level — silent shadowing is easy to miss in cog.yaml diffs. - Show "(+N more)" indicator in cog weights status for multi-source weights so the truncation is visible. - Render multi-source plan output on multiple lines instead of joining URIs with commas (URLs can contain commas). - Drop the unused CombineFingerprints helper and merged.Fingerprint field; per-source fingerprints are tracked separately and used wherever drift matters.

Every existing weight test instantiated a single-element source list, leaving the multi-source merge, override, and per-source fingerprint paths exercised only in production. Add tests for: - merging disjoint files from two sources into one inventory and resolving overlapping paths last-in-wins (table-driven, since the two cases share setup shape) - per-source fingerprint drift (mutating one source leaves the other's fingerprint stable) - PlanImport surfacing drift on a specific source index - YAML and JSON parsing of the array form of source: - MarshalYAML/MarshalJSON shape contract (mapping for single, sequence/array for multi) with round-trip through unmarshal - validateWeights accepting the multi-source form and attributing pattern errors to the right source index The new tests live alongside the existing single-source tests they parallel — multi-source isn't a separate concept, just a different length of the source list.

ask-bonk · 2026-05-07T20:04:20Z

I'm Bonk, and I've done a quick review of your PR.

Summary of changes: Adds multi-source weight support with last-in-wins merging and HTTPS/HTTP weight sources with ETag-based fingerprinting.

Issues found:

Test coverage gap: The HTTP source handles weak ETags (RFC 7232 §2.3) by ignoring them and falling back to SHA256 hashing, but this behavior isn't tested. The implementation in http.go:199-201 correctly handles W/"..." prefixes, but http_test.go lacks a test case for this scenario.

func TestHTTPSource_Inventory_WeakETag_FallsBackToSHA256(t *testing.T) {
	body := []byte("fake model weights")
	h := sha256.Sum256(body)
	expectedDigest := "sha256:" + hex.EncodeToString(h[:])

	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		w.Header().Set("ETag", `W/"weak-etag-123"`)
		w.Header().Set("Content-Length", "18")
		if r.Method == http.MethodHead {
			w.WriteHeader(http.StatusOK)
			return
		}
		w.WriteHeader(http.StatusOK)
		_, _ = w.Write(body)
	}))
	defer srv.Close()

	src, err := NewHTTPSource(srv.URL + "/model.pth")
	require.NoError(t, err)

	inv, err := src.Inventory(context.Background())
	require.NoError(t, err)

	require.Len(t, inv.Files, 1)
	assert.Equal(t, expectedDigest, inv.Files[0].Digest)
	assert.Equal(t, Fingerprint(expectedDigest), inv.Fingerprint,
		"weak ETag should be ignored, falling back to sha256 fingerprint")
}

github run

markphelps · 2026-05-07T22:36:44Z

+// that NewHTTPSource and normalizeHTTPURI cannot drift apart. The
+// "no userinfo" rule exists because the URI lands in weights.lock —
+// see NewHTTPSource for the full rationale.
+func parseHTTPURI(uri string) (*url.URL, error) {


Query-string credentials like presigned S3/GCS/GitHub tokens are accepted and can be written to weights.lock or shown in output. Fix by separating fetch URL from persisted/display URL and redacting or rejecting credential-bearing query params.

good point! I don't think we should block on that, but worth coming back to. I'll make an issue!

markphelps · 2026-05-07T22:37:59Z

+	if err != nil {
+		return nil, fmt.Errorf("invalid HTTP URI %q: %w", uri, err)
+	}
+	if parsed.Scheme != "https" && parsed.Scheme != "http" {


should we instead default to only allow HTTPS with maybe insecure opt-in for localhost/dev cases?

not sure what we'd gain by restricting it atm. we aren't running untrusted code, so there's not the same risk as eg docker where spoofing or mitm could serve different content. this is just fetching and unpacking blobs

markphelps

lgtm!!

Add Go fuzz tests for config parsing (FuzzParseBytes, FuzzFromYAML, FuzzConfigComplete) and fix two categories of crashes found: 1. version.Matches/Equal/Greater panicked on invalid version strings like "11.A". Replace MustVersion with graceful NewVersion handling. 2. yaml v4 LoadErrors created infinite recursion when errors.Is traversed wrapped errors. Use %v instead of %w when wrapping yaml parse errors to break the self-referential cycle. Also validate CUDA minor version in validateCudaVersion().

ask-bonk · 2026-05-08T16:26:44Z

I'm Bonk, and I've done a quick review of your PR.

Summary of changes: Adds multi-source weight support with last-in-wins merging and HTTPS/HTTP weight sources with ETag-based fingerprinting.

Issues found:

Test coverage gap: The HTTP source handles weak ETags (RFC 7232 §2.3) by ignoring them and falling back to SHA256 hashing, but this behavior isn't tested. The implementation in http.go:199-201 correctly handles W/"..." prefixes, but http_test.go lacks a test case for this scenario.

func TestHTTPSource_Inventory_WeakETag_FallsBackToSHA256(t *testing.T) {
	body := []byte("fake model weights")
	h := sha256.Sum256(body)
	expectedDigest := "sha256:" + hex.EncodeToString(h[:])

	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		w.Header().Set("ETag", `W/"weak-etag-123"`)
		w.Header().Set("Content-Length", "18")
		if r.Method == http.MethodHead {
			w.WriteHeader(http.StatusOK)
			return
		}
		w.WriteHeader(http.StatusOK)
		_, _ = w.Write(body)
	}))
	defer srv.Close()

	src, err := NewHTTPSource(srv.URL + "/model.pth")
	require.NoError(t, err)

	inv, err := src.Inventory(context.Background())
	require.NoError(t, err)

	require.Len(t, inv.Files, 1)
	assert.Equal(t, expectedDigest, inv.Files[0].Digest)
	assert.Equal(t, Fingerprint(expectedDigest), inv.Fingerprint,
		"weak ETag should be ignored, falling back to sha256 fingerprint")
}

github run

ask-bonk

I'm Bonk, and I've done a quick review of your PR.

Summary of changes: Adds multi-source weight support with last-in-wins merging and HTTPS/HTTP weight sources with ETag-based fingerprinting.

Issues found:

Test coverage gap: The HTTP source handles weak ETags (RFC 7232 §2.3) by ignoring them and falling back to SHA256 hashing, but this behavior isn't tested. The implementation in http.go:199-201 correctly handles W/"..." prefixes, but http_test.go lacks a test case for this scenario.

ask-bonk · 2026-05-08T16:29:23Z

Posted a review comment on http_test.go highlighting the missing test coverage for weak ETag handling. The implementation correctly ignores weak ETags (lines 199-201), but there's no test verifying this behavior falls back to SHA256 fingerprinting.

github run

ask-bonk · 2026-05-08T16:52:29Z

LGTM

github run

michaeldwan force-pushed the md/gh-releases-multi-file-sources branch from 6929793 to dbd9722 Compare May 7, 2026 19:09

michaeldwan added 3 commits May 7, 2026 13:58

michaeldwan force-pushed the md/gh-releases-multi-file-sources branch from dbd9722 to 8a67b5a Compare May 7, 2026 20:00

michaeldwan marked this pull request as ready for review May 7, 2026 20:02

michaeldwan requested a review from a team as a code owner May 7, 2026 20:02

markphelps reviewed May 7, 2026

View reviewed changes

markphelps previously approved these changes May 8, 2026

View reviewed changes

michaeldwan dismissed markphelps’s stale review via 09ba90b May 8, 2026 16:25

Merge branch 'main' into md/gh-releases-multi-file-sources

8b899b6

ask-bonk Bot reviewed May 8, 2026

View reviewed changes

Comment thread pkg/model/weightsource/http_test.go

Test weak ETag fallback to SHA256 in HTTP source

b8bf120

michaeldwan merged commit 14d224e into main May 8, 2026
42 checks passed

michaeldwan deleted the md/gh-releases-multi-file-sources branch May 8, 2026 17:53

Conversation

michaeldwan commented May 7, 2026

Notes for reviewers

Uh oh!

ask-bonk Bot commented May 7, 2026

Uh oh!

ask-bonk Bot commented May 7, 2026

Uh oh!

ask-bonk Bot commented May 7, 2026

Uh oh!

markphelps May 7, 2026

Choose a reason for hiding this comment

Uh oh!

michaeldwan May 8, 2026

Choose a reason for hiding this comment

Uh oh!

markphelps May 7, 2026

Choose a reason for hiding this comment

Uh oh!

michaeldwan May 8, 2026

Choose a reason for hiding this comment

Uh oh!

markphelps left a comment

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot commented May 8, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ask-bonk Bot commented May 8, 2026

Uh oh!

ask-bonk Bot commented May 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants