Skip to content

[Bugfix] fix spec/label/annotation changes can't trigger redownload#552

Open
pallasathena92 wants to merge 1 commit intomainfrom
yifeliu/fix-redownload
Open

[Bugfix] fix spec/label/annotation changes can't trigger redownload#552
pallasathena92 wants to merge 1 commit intomainfrom
yifeliu/fix-redownload

Conversation

@pallasathena92
Copy link
Copy Markdown
Collaborator

@pallasathena92 pallasathena92 commented Mar 30, 2026

What this PR does

Replaces shouldDownloadModelInUpdateEvent (which used defaultDecision=false) with shouldDownloadModel (which uses defaultDecision=true). When a model has spec/label/annotation changes AND has no NodeSelector or NodeAffinity, it will now trigger generateDownloadOverrideTask.

Why we need it

What shouldDownloadModelCommon does:

  1. storageSpec == nil → return true
  2. PVC storage → return false
  3. NodeSelector doesn't match this node → return false
  4. NodeAffinity doesn't match this node → return false
  5. Nothing rejected it → return defaultDecision
    The key difference:

shouldDownloadModel passes defaultDecision = true → "yes, download unless something explicitly rejects it"
shouldDownloadModelInUpdateEvent passes defaultDecision = false → "no, don't download unless... nothing can make it true"

With this function, we can't trigger model redownload whtn the model has spec/label/annotation changes.

What changes:

  • add policyChanged check in updateClusterBaseModel and updateBaseModel to check if downloadPolicy update need to trigger downloadOverrideTask
    • current isToDownloadOverrideDueToDownloadPolicyBasedOnBM is scoped to HuggingFace only. will refactor this when we support download policy in all storageType.
  • add ignoreDownloadPolicy when spec/label/annotation changes check.
  • combine these two checks before downloadOverrideTask generation to avoid redundant task generation.

How to test

Checklist

  • Tests added/updated (if applicable)
  • Docs updated (if applicable)
  • make test passes locally

@github-actions github-actions bot added the model-agent Model agent changes label Mar 30, 2026
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request simplifies the model download logic by replacing shouldDownloadModelInUpdateEvent with shouldDownloadModel and removing the deprecated function. The review feedback identifies that this change introduces redundancy in the update logic for both BaseModel and ClusterBaseModel, potentially resulting in duplicate download tasks when the download policy is modified.

@pallasathena92 pallasathena92 force-pushed the yifeliu/fix-redownload branch from 321047e to 8e1db1d Compare March 30, 2026 23:54
@github-actions github-actions bot added the tests Test changes label Mar 30, 2026
@heymrbox heymrbox self-requested a review April 3, 2026 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

model-agent Model agent changes tests Test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants