-
Notifications
You must be signed in to change notification settings - Fork 48
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
Fix the compiled pipeline compilation issue #798
base: main
Are you sure you want to change the base?
Conversation
bump-punet-staging is a somewhat old branch. Would be better to get a cloned branch of bump-punet (can't mess with bump-punet due to mlperf submission) and merge submit PR with that |
6f6585b
to
878449e
Compare
7504c6c
to
121a409
Compare
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.
Can you work on getting this merged into main instead of a separate compiled-pipeline branch? Would need to allow for flag controlling whether to use compile-pipeline or not
@@ -465,13 +465,10 @@ def encode_prompts_sdxl(self, prompt, negative_prompt): | |||
text_input_ids_list += text_inputs.input_ids.unsqueeze(0) | |||
uncond_input_ids_list += uncond_input.input_ids.unsqueeze(0) | |||
|
|||
if self.compiled_pipeline: |
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.
Is there any particular reason you are removing the non compiled_pipeline option 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.
It's removing the compiled_pipeline version - I think this is a relic of previous iterations when the text encoder was going to be part of the pipeline, but since it isn't we can run the same code for compiled and python pipelines.
@@ -624,9 +622,11 @@ def produce_images_compiled( | |||
sample, | |||
prompt_embeds, | |||
text_embeds, | |||
guidance_scale, | |||
torch.as_tensor([guidance_scale], dtype=sample.dtype), |
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.
Is this necessary?
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.
Yep, the compiled pipeline expects an fp16
tensor while passing the raw value produces an fp64
one.
@@ -36,13 +36,13 @@ | |||
], | |||
"unet": [ | |||
"--iree-flow-enable-aggressive-fusion", | |||
"--iree-flow-enable-fuse-horizontal-contractions=true", | |||
"--iree-global-opt-enable-fuse-horizontal-contractions=true", |
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.
Is change needed for current IREE?
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.
It seems to be, at least as of the 7/24 nightly. We might be using a newer version in the CI.
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.
7/24 is very old. Should test with latest or at least what iree-turbine pulls (which of right now is iree-compiler==20240828.999). Looks like the CI failures you are seeing are also due to flag changes with newer iree versions
121a409
to
1d71f8c
Compare
It compiles the vmfb now, but there are some issues in the script preventing it from running properly. I can fix that in this PR as well or make a new one after this one is merged.