From 1df5a3ef7fc166a64133960b142ad921bbfcbd4c Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Thu, 23 Jul 2026 16:34:56 +0000 Subject: [PATCH] fix(model-artifacts): keep the companion option present on a remote load, even unresolved Follow-up to the companion-persistence fix. On a live distributed cluster the managed base_model companion option still failed to reach the remote worker (nvidia-thor) even with the companion resolved-and-persisted in the config: the backend logged "Downloading required files for meituan-longcat/LongCat-Video" and failed "base_model must point to a LongCat-Video checkpoint". Symlinking the companion into place did not help (the option was simply absent from the worker's LoadModel), while an explicit absolute base_model in options: worked as a control. Trace of where the remote ModelOptions is built and whether the companion is present there: - The *pb.ModelOptions the worker's LoadModel consumes is built on the CONTROLLER by grpcModelOpts (core/backend/options.go) -> withCompanionArtifactOptions, set as gRPCOptions, and sent by direct gRPC via FileStagingClient.LoadModel. It is NOT rebuilt on the worker. The reconciler's replica scale-up instead replays a Postgres-stored proto blob, which already carries whatever grpcModelOpts produced. - withCompanionArtifactOptions is the ONLY builder of ModelOptions.Options in the tree, and it emits base_model iff the config's companion artifact has Resolved != nil. Staging preserves the option and derives the worker ModelPath as the nested per-model staged root, so a resolved companion resolves under it without a download (verified end to end; the path-nesting angle is a red herring here). So the option is absent only when the config the loader is serving from carries the companion WITHOUT a resolved snapshot (its resolved state not reaching the serving config, e.g. a peer-replica reload from local disk or a config loaded before resolution). In that state the old code emitted NOTHING for the companion, and longcat-video fell back to its OWN hardcoded default (BASE_MODEL_ID), which is exactly the observed download-and-fail. Fix: an unresolved-but-declared companion no longer vanishes. It now falls back to its DECLARED source repository id, so the backend fetches the artifact the config actually asked for instead of a hardcoded default; the resolved snapshot path (the staged, no-download fast path) is still preferred whenever the companion is resolved, so the single-node and healthy distributed paths are unchanged. The fallback logs a warning naming the artifact and repo, and the router now logs the exact option strings crossing to the worker at debug, so a recurrence is diagnosable in one load instead of by inference. Assisted-by: Claude Code:claude-opus-4-8[1m] [Read] [Edit] [Bash] Signed-off-by: Ettore Di Giacinto --- core/backend/companion_options_test.go | 29 +++++++++++++++-- core/backend/options.go | 44 +++++++++++++++++++++++--- core/services/nodes/router.go | 9 ++++++ 3 files changed, 74 insertions(+), 8 deletions(-) diff --git a/core/backend/companion_options_test.go b/core/backend/companion_options_test.go index c4b6f3953..123fa597c 100644 --- a/core/backend/companion_options_test.go +++ b/core/backend/companion_options_test.go @@ -113,11 +113,34 @@ var _ = Describe("companion artifact backend options", func() { Expect(opts.Options).To(Equal([]string{"attention_backend:sdpa"})) }) - It("skips a companion that has not been resolved yet", func() { + It("names the source repository when the companion is not resolved yet", func() { + // A companion that reaches load time WITHOUT a resolved snapshot must not + // vanish silently: emitting no option lets the backend fall back to its own + // hardcoded default, which is how a distributed longcat-video worker ended + // up trying to load the wrong base model and failing "base_model must point + // to a LongCat-Video checkpoint". Naming the DECLARED repository instead + // points the backend at the artifact the config actually asked for. The + // snapshot path (the staged, no-download fast path) is still preferred + // whenever the companion IS resolved. cfg := configWithCompanion() cfg.Artifacts[1].Resolved = nil opts := grpcModelOpts(cfg, "/models") - _, found := optionValue(opts.Options, "base_model") - Expect(found).To(BeFalse()) + + value, found := optionValue(opts.Options, "base_model") + Expect(found).To(BeTrue()) + Expect(value).To(Equal("meituan-longcat/LongCat-Video")) + // The fallback is a repo reference, never a models-relative snapshot path. + Expect(value).ToNot(ContainSubstring(".artifacts")) + }) + + It("prefers the resolved snapshot path over the source repository", func() { + opts := grpcModelOpts(configWithCompanion(), "/models") + value, found := optionValue(opts.Options, "base_model") + Expect(found).To(BeTrue()) + expected, err := modelartifacts.RelativeSnapshotPath(companionKey) + Expect(err).NotTo(HaveOccurred()) + Expect(value).To(Equal(expected)) + // The resolved fast path must never degrade to a bare repo id. + Expect(value).ToNot(Equal("meituan-longcat/LongCat-Video")) }) }) diff --git a/core/backend/options.go b/core/backend/options.go index 72f49f1ed..816f8e4a5 100644 --- a/core/backend/options.go +++ b/core/backend/options.go @@ -294,6 +294,13 @@ func EffectiveBatchSize(c config.ModelConfig) int { // // An option the author set explicitly always wins: pinning a companion to a // local checkout has to beat the managed snapshot. +// +// A companion that is declared but NOT resolved falls back to its source +// repository id rather than being dropped: a dropped companion is invisible to +// the backend, which then loads its own hardcoded default and fails far away +// from the cause. The repo-id fallback trades the staging fast path (the weights +// are fetched on the worker) for correctness, and logs a warning so the missing +// controller-side resolution is diagnosable. func withCompanionArtifactOptions(options []string, artifacts []modelartifacts.Spec) []string { configured := make(map[string]struct{}, len(options)) for _, option := range options { @@ -306,19 +313,46 @@ func withCompanionArtifactOptions(options []string, artifacts []modelartifacts.S // reallocate away from) the config's own slice. combined := slices.Clone(options) for _, artifact := range artifacts { - if artifact.Target != modelartifacts.TargetCompanion || artifact.Resolved == nil { + if artifact.Target != modelartifacts.TargetCompanion { continue } if _, exists := configured[artifact.Name]; exists { xlog.Debug("keeping the configured companion option over the managed snapshot", "artifact", artifact.Name) continue } - snapshot, err := modelartifacts.RelativeSnapshotPath(artifact.Resolved.CacheKey) - if err != nil { - xlog.Warn("skipping companion artifact with an unusable cache key", "artifact", artifact.Name, "error", err) + + // Preferred fast path: a resolved companion is surfaced as its staged, + // models-relative snapshot directory. Staging materializes exactly this + // path on a remote worker and the backend resolves it under its own + // ModelPath, so the weights are never fetched again at load time. + if artifact.Resolved != nil { + if snapshot, err := modelartifacts.RelativeSnapshotPath(artifact.Resolved.CacheKey); err == nil { + xlog.Debug("surfacing resolved companion snapshot to the backend", "artifact", artifact.Name, "path", snapshot) + combined = append(combined, artifact.Name+":"+snapshot) + continue + } else { + xlog.Warn("companion artifact has an unusable cache key; falling back to its source repository", "artifact", artifact.Name, "error", err) + } + } + + // Fallback: the companion reached load time without a resolved snapshot + // (its resolved state never made it into the config the loader is serving + // from, e.g. after a controller restart or a peer-replica config reload). + // Emitting nothing here is what makes the failure so hard to see: the + // backend then falls back to its OWN hardcoded default companion, which on + // a distributed longcat-video worker meant fetching the wrong base model + // and failing "base_model must point to a LongCat-Video checkpoint". Name + // the DECLARED repository instead, so the backend at least fetches the + // artifact the config actually asked for. It is a warn because it means the + // no-download fast path was lost: the controller-side materialization or + // persistence for this companion needs investigating. + if repo := strings.TrimSpace(artifact.Source.Repo); repo != "" { + xlog.Warn("companion artifact is not resolved on the controller; the backend will fetch it by repository id (no staging fast path)", + "artifact", artifact.Name, "repo", repo) + combined = append(combined, artifact.Name+":"+repo) continue } - combined = append(combined, artifact.Name+":"+snapshot) + xlog.Warn("companion artifact is neither resolved nor has a source repository; the backend will get no option for it", "artifact", artifact.Name) } return combined } diff --git a/core/services/nodes/router.go b/core/services/nodes/router.go index 5b6aed953..d9bf3a749 100644 --- a/core/services/nodes/router.go +++ b/core/services/nodes/router.go @@ -342,6 +342,15 @@ func (r *SmartRouter) scheduleAndLoad(ctx context.Context, backendType, tracking xlog.Info("Loading model on remote node", "node", node.Name, "model", modelName, "addr", backendAddr, "payloadBytes", payloadBytes, "loadBudget", loadTimeout) + // The exact option strings that cross to the worker. A managed companion + // (e.g. longcat-video's base_model) rides here as a key:value option, and + // its absence is otherwise invisible until the backend fails far away + // having fetched the wrong weights. Logged at debug so a load that + // "downloaded the base model" can be traced to whether the option was + // present in what the worker actually received. + xlog.Debug("Remote LoadModel options", "node", node.Name, "model", modelName, + "options", loadOpts.Options, "modelPath", loadOpts.ModelPath, "modelFile", loadOpts.ModelFile) + // The cold-load hold above this call extends on STAGING progress, and // the remote LoadModel reports none — so once the last byte lands the // hold expires a stall window later and would cancel a load that is