Skip to content
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

Feat (ui): Add VAE Model to Recall Parameters #5073

Merged
merged 6 commits into from
Nov 12, 2023

Conversation

StefanTobler
Copy link
Contributor

@StefanTobler StefanTobler commented Nov 11, 2023

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update
  • Community Node Submission

Have you discussed this change with the InvokeAI team?

  • Yes
  • No, because: it is 2 am est

#4796

Have you updated all relevant documentation?

  • Yes
  • No

I don't believe there is any documentation explicitly referencing the recall parameters.

Description

This code changes allows users to recall the VAE model as a Recall Parameter. The VAE model is now part of the "recall all parameters" setting. There is room to add the VAE Precision to the metadata and also making that a recall parameter..

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

  1. Generate an image using a VAE other than default.
  2. Select that generated image, change the VAE model, and select "Use All"
  3. VAE model should change in the UI.
  4. VAE model is available in "recall parameters" of the metadata section.
vae_recall.mp4

Added/updated tests?

  • Yes
  • No : no frontend test suite

[optional] Are there any post deployment tasks we need to perform?

@StefanTobler StefanTobler changed the title Adding VAE Model to Recall Parameters Feat (ui): Add VAE Model to Recall Parameters Nov 11, 2023
@StefanTobler
Copy link
Contributor Author

StefanTobler commented Nov 11, 2023

This PR may not be considered "complete" as it does not recall the "default" VAE model if that is selected. I think that this would need the addition of adding the default VAE to the metadata, as of right now, if the VAE model is default it is not added in the generated images metadata. Sorry tired and I was not thinking, ofc I can add this, just check for a null VAE. I can add this tomorrow.

@StefanTobler
Copy link
Contributor Author

Last note for tonight, I also noticed that the naming for VAE is a bit inconsistent around the frontend, it seems it is either VAE or VAE Model, but they both refer to the same thing.

@psychedelicious
Copy link
Collaborator

Yep as you noted, lets add some additional logic to check if the vae is nullish when recalling all, and if so, set it to null.

Looking good otherwise!

Stretch goal: check the RTK Query cache when recalling the VAE to ensure it is actually available on the system. Happy to have this be a future change, and we should do the same for when we recall a main model.

@StefanTobler
Copy link
Contributor Author

@psychedelicious added the check for nullish variables. It is a bit verbose imo, but I am not sure if it is worth making it harder to read to reduce the code.

Also debating on adding similar logic to ImageMetadataActions.tsx so that vae is displayed as "Default" when nullish. I am ignorant as to when this would be used in the event that VAE is not present for a reason. I guess it seems strange to me to be making all the exceptions for this one field but not all the others when null. Maybe overthinking.

@StefanTobler
Copy link
Contributor Author

I guess it lead me to question, if the VAE is undefined, it is okay to assume we oughta force the default VAE model on the parameters?

@StefanTobler
Copy link
Contributor Author

image

@Millu
Copy link
Contributor

Millu commented Nov 12, 2023

I guess it lead me to question, if the VAE is undefined, it is okay to assume we oughta force the default VAE model on the parameters?

This behavior makes sense to me

@StefanTobler
Copy link
Contributor Author

Agreed, I added that in the latest commit.

Copy link
Collaborator

@psychedelicious psychedelicious left a comment

Choose a reason for hiding this comment

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

Thanks @StefanTobler !

@psychedelicious psychedelicious enabled auto-merge (squash) November 12, 2023 21:18
@psychedelicious psychedelicious merged commit 71e298b into invoke-ai:main Nov 12, 2023
8 checks passed
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.

[bug]: VAE is not changing when "Use all"
3 participants