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

Add API Endpoints [POC] #611

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Add API Endpoints [POC] #611

wants to merge 9 commits into from

Conversation

ArrowM
Copy link

@ArrowM ArrowM commented Aug 27, 2023

I would like to add endpoints for generating prompts. I found "Don't generate images" to be clunky. I created a basic one that covers my use case, but it could easily be expanded to cover the other ~15 params. The biggest change here is splitting some of the process() code into a separate method for generating all_prompts and negative_prompts
New endpoint can be found at http://127.0.0.1:7860/docs#/default/evaluate_dynamicprompts_evaluate_post

Copy link
Collaborator

@akx akx left a comment

Choose a reason for hiding this comment

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

Nice idea! The PR needs some cleaning up, though; see within, please :)

scripts/api.py Outdated
@@ -0,0 +1,46 @@
import gradio as gr
Copy link
Collaborator

Choose a reason for hiding this comment

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

As much as possible should be in the sd_dynamic_prompts package, please.

Copy link
Author

Choose a reason for hiding this comment

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

Moved.

scripts/api.py Outdated Show resolved Hide resolved
scripts/api.py Outdated Show resolved Hide resolved
scripts/api.py Outdated Show resolved Hide resolved
sd_dynamic_prompts/dynamic_prompting.py Outdated Show resolved Hide resolved
Comment on lines 358 to 377
self,
p,
is_enabled: bool,
is_combinatorial: bool,
combinatorial_batches: int,
is_magic_prompt: bool,
is_feeling_lucky: bool,
is_attention_grabber: bool,
min_attention: float,
max_attention: float,
magic_prompt_length: int,
magic_temp_value: float,
use_fixed_seed: bool,
unlink_seed_from_prompt: bool,
disable_negative_prompt: bool,
enable_jinja_templates: bool,
no_image_generation: bool,
max_generations: int,
magic_model: str | None,
magic_blocklist_regex: str | None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please revert the spurious formatting changes here :)

Copy link
Author

Choose a reason for hiding this comment

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

Reverted.

original_negative_prompt,
)

def generate_prompts(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just generating prompts shouldn't require the full p object, I don't think..?

Further, maybe this should be made a free function, and it should probably be kwarg-only ((*, original_prompt...)) because otherwise it's super easy to pass arguments in the wrong order.

Copy link
Author

@ArrowM ArrowM Aug 27, 2023

Choose a reason for hiding this comment

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

p is used for its additional seed settings. see around dynamic_prompting.py.py#558 and around dynamic_prompting.py.py#572.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, but those probably ought to be moved on from this function?

Copy link
Author

@ArrowM ArrowM Aug 27, 2023

Choose a reason for hiding this comment

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

I'm not seeing an easy way to move them out, any suggestions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, GeneratorBuilder().set_context(p) is a bit problematic, but that's only used for Jinja context – the p-to-context code in create_jinja_generator should be refactored out. (I think Jinja over the API would fail right now since p.sd_model will not get set.)

With that done, I the p.all_seeds, p.all_subseeds = get_seeds(...) manipulation could be done outside the prompt-generation function, couldn't it?

@ArrowM ArrowM requested a review from akx August 27, 2023 18:43
@ArrowM
Copy link
Author

ArrowM commented Aug 31, 2023

@akx Hey, I am still looking for review feedback. I think I addressed everything except removing p from generate_prompts (which I don't think can be done cleanly because it's used in a few places). lmk what you think

@akx
Copy link
Collaborator

akx commented Sep 11, 2023

@ArrowM ICYMI, I replied to the review comment :)

@ArrowM
Copy link
Author

ArrowM commented Sep 30, 2023

@akx Hey, sorry for the really delayed response. I don't plan to dig into any of the p context code to refactor this further. I'll leave this to you to either run with it or close the PR. Thanks for your work on the extension! <3

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.

2 participants