-
Notifications
You must be signed in to change notification settings - Fork 289
Refactor CLIP
and update SD3.
#2316
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
# TODO: Deprecate this in favor of | ||
# `keras.layers.LayerNormalization(rms_scaling=True)` once we require Keras | ||
# 3.10 or later. | ||
class RMSNormalization(layers.Layer): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this, can you try using this layer: https://github.com/keras-team/keras/blob/master/keras/src/layers/normalization/rms_normalization.py#L7?
keras.layers.LayerNormalization(rms_scaling=True)
was incorrectly implemented, but keras.layers.RMSNormalization
is correct. It should work here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, keras.layers.RMSNormalization
requires keras >= 3.9.
I have added an identical custom layer as a fallback if layers
doesn't include it.
@abheesht17 Using JAX + CUDA:
EDITED: |
@james77777778 we have error tolerance error with TF GPU, can you please check? |
/gemini review |
There was a problem hiding this 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 aims to align the Keras implementation of Stable Diffusion 3 with the diffusers
library by updating the layer normalization, scheduler, and text encoder data types. The changes are extensive, touching model definitions, backbones, and the checkpoint conversion script. The introduction of a custom RMSNormalization
layer for compatibility and the refactoring of the scheduler logic are significant improvements. The conversion script is also updated to perform numerical validation against diffusers
, which is a great step towards ensuring correctness.
My review focuses on improving code clarity and maintainability. I've identified a few opportunities to refactor repetitive code blocks and clarify stateful logic in a custom layer. These changes should make the code easier to read and manage in the future. Overall, this is a solid contribution toward improving the model's fidelity with the reference implementation.
keras_hub/src/models/stable_diffusion_3/stable_diffusion_3_backbone.py
Outdated
Show resolved
Hide resolved
d48544e
to
49743e9
Compare
49743e9
to
df3f364
Compare
In this PR:
Caution: We need to upload the updated presets for CLIP and SD3 after the merge. |
LayerNormalization
, scheduler and the dtype of the text encoders.CLIP
and update SD3.
Description of the change
Hi team,
I tried to add a numeric check for SD3 but was unsuccessful. It seems that the output latent differs slightly from the
diffusers
impl.Here’s what I tried:
LayerNormalization
with the custom layer which is the same asdiffusers
's impl.diffusers
's impl.diffusers
's pipeline andkeras_hub
's pipeline.diffusers.StableDiffusion3Pipeline.encode_prompt
to get the text embeddings and fed them tokeras_hub.models.StableDiffusion3TextToImage
diffusers
's impl. (timesteps
,sigma
, text embeddings and input latent)denoise_step
to verify the latent numerics.The latent numerics failed to pass the check with
atol=1e-1
.However, the generated images look good.
stable_diffusion_3.5_medium
stable_diffusion_3.5_large
stable_diffusion_3.5_large_turbo
cc @abheesht17
I think this PR still helps narrow the gap between
keras_hub
's impl anddiffusers
's impl.Uploading new presets is required.
Reference
Colab Notebook
Checklist