Skip to content

Add format dduf#793

Merged
ilopezluna merged 3 commits intomainfrom
add-format-dduf
Mar 30, 2026
Merged

Add format dduf#793
ilopezluna merged 3 commits intomainfrom
add-format-dduf

Conversation

@ilopezluna
Copy link
Copy Markdown
Contributor

This pull request updates the handling of model formats by introducing a new FormatDDUF type to replace the previously used FormatDiffusers, which is now deprecated but retained for backward compatibility.

Diffusers is not a format, so using it as config.FormatDiffusers is missleading.

@ilopezluna ilopezluna requested a review from a team March 26, 2026 09:34
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've left some high level feedback:

  • You now special-case FormatDiffusers for backward compatibility in several places with //nolint:staticcheck; consider centralizing this behavior (e.g., a helper or shared constant/list) so the deprecation handling is defined in one place instead of being duplicated across client, unpack, backend resolution, and model format parsing.
  • Where you accept both FormatDDUF and FormatDiffusers (e.g., GetSupportedFormats, backendFromFormat, unpackLegacy), double-check that the ordering and precedence are consistent with the new semantics—that DDUF is the primary format and Diffusers is truly treated as a legacy alias—to avoid subtle behavior differences between call sites.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- You now special-case `FormatDiffusers` for backward compatibility in several places with `//nolint:staticcheck`; consider centralizing this behavior (e.g., a helper or shared constant/list) so the deprecation handling is defined in one place instead of being duplicated across client, unpack, backend resolution, and model format parsing.
- Where you accept both `FormatDDUF` and `FormatDiffusers` (e.g., `GetSupportedFormats`, `backendFromFormat`, `unpackLegacy`), double-check that the ordering and precedence are consistent with the new semantics—that DDUF is the primary format and Diffusers is truly treated as a legacy alias—to avoid subtle behavior differences between call sites.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

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 introduces a new distinct format, FormatDDUF, for Diffusers files, separating it from the existing FormatDiffusers. The FormatDiffusers is now deprecated but maintained for backward compatibility across various components, including format detection, backend resolution, and supported format lists. This clarifies format identification and handling for DDUF files while ensuring existing models continue to function. There are no review comments to address.

# Conflicts:
#	pkg/distribution/distribution/client.go
Copy link
Copy Markdown
Contributor

@doringeman doringeman left a comment

Choose a reason for hiding this comment

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

LGTM, with one question: shouldn't DDUF be handled in func (s *Scheduler) selectBackendForModel as well?

@ilopezluna
Copy link
Copy Markdown
Contributor Author

LGTM, with one question: shouldn't DDUF be handled in func (s *Scheduler) selectBackendForModel as well?

Yes! thanks, I'm going to add it in a follow-up PR

@ilopezluna ilopezluna merged commit 8996da2 into main Mar 30, 2026
15 checks passed
@ilopezluna ilopezluna deleted the add-format-dduf branch March 30, 2026 09:51
@ilopezluna ilopezluna mentioned this pull request Mar 30, 2026
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.

2 participants