Conversation
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- You now special-case
FormatDiffusersfor 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
FormatDDUFandFormatDiffusers(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.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
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
doringeman
left a comment
There was a problem hiding this comment.
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 |
This pull request updates the handling of model formats by introducing a new
FormatDDUFtype to replace the previously usedFormatDiffusers, which is now deprecated but retained for backward compatibility.Diffusers is not a format, so using it as
config.FormatDiffusersis missleading.