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

Maryhipp/trigger phrases #5825

Closed
wants to merge 7 commits into from
Closed

Maryhipp/trigger phrases #5825

wants to merge 7 commits into from

Conversation

maryhipp
Copy link
Collaborator

@maryhipp maryhipp commented Feb 28, 2024

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:

Description

  • Update model manager to allow trigger phrase management as part of model metadata (subject to change with @psychedelicious 's schema rework)
  • Adapt Embedding popover on prompts to work for trigger phrases, as well. Now only embeddings that are compatible with base model will be displayed alongside trigger phrases configured for that selected model.

QA Instructions, Screenshots, Recordings

Screenshot 2024-02-28 at 12 07 07 PM

Screenshot 2024-02-28 at 12 07 33 PM

Merge Plan

Added/updated tests?

  • Yes
  • No : please replace this line with details on why tests
    have not been included

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

@github-actions github-actions bot added api python PRs that change python files backend PRs that change backend files services PRs that change app services frontend PRs that change frontend files labels Feb 28, 2024
@@ -164,6 +164,7 @@ def _from_model_json(self, model_json: Dict[str, Any], version_id: Optional[int]
AllowDerivatives=model_json["allowDerivatives"],
AllowDifferentLicense=model_json["allowDifferentLicense"],
),
trigger_phrases=version_json["trainedWords"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is already on the metadata object as trained_words

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I was trying to not use the civit key just to spite them

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean the metadata schema already extracts it a few lines earlier

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you prefer trained_words? I don't feel strongly

Copy link
Collaborator

Choose a reason for hiding this comment

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

trigger_phrases works for me, and is more consistent with the terminology we use for textual inversion embeddings.

@maryhipp maryhipp mentioned this pull request Feb 29, 2024
10 tasks
Copy link
Collaborator

@lstein lstein left a comment

Choose a reason for hiding this comment

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

The MM backend changes look fine to me.

@@ -164,6 +164,7 @@ def _from_model_json(self, model_json: Dict[str, Any], version_id: Optional[int]
AllowDerivatives=model_json["allowDerivatives"],
AllowDifferentLicense=model_json["allowDifferentLicense"],
),
trigger_phrases=version_json["trainedWords"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

trigger_phrases works for me, and is more consistent with the terminology we use for textual inversion embeddings.

Copy link
Collaborator

@lstein lstein left a comment

Choose a reason for hiding this comment

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

The backend changes look fine to me. I did limited testing of the frontend due to the Civitai license restrictions schema change causing Civitai installs to fail, but adding and removing trigger phrases from the list seems to work.

@maryhipp maryhipp changed the base branch from next to main March 1, 2024 13:58
@maryhipp maryhipp requested a review from ebr as a code owner March 1, 2024 13:58
@maryhipp maryhipp changed the base branch from main to next March 1, 2024 14:54
@maryhipp maryhipp changed the base branch from next to main March 1, 2024 15:13
@maryhipp maryhipp changed the base branch from main to next March 1, 2024 15:41
@maryhipp maryhipp closed this Mar 1, 2024
@maryhipp maryhipp deleted the maryhipp/trigger-phrases branch March 1, 2024 15:47
@maryhipp maryhipp mentioned this pull request Mar 1, 2024
10 tasks
@maryhipp maryhipp restored the maryhipp/trigger-phrases branch March 4, 2024 19:56
@maryhipp maryhipp deleted the maryhipp/trigger-phrases branch March 4, 2024 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api backend PRs that change backend files frontend PRs that change frontend files python PRs that change python files services PRs that change app services
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants