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

Copy prompt parameters from custom workflows #1429

Merged
merged 1 commit into from
Nov 25, 2024

Conversation

ronan36880
Copy link
Contributor

This change makes it possible to also copy the parameters used in custom workflows. It works by going through all the metadata in the job and sets the matching widget's value.

This makes it so "Copy prompt" will now also work on custom workflows and automatically sets the values on the UI.

I had to implement a signal, but I'm unsure if it's the best approach to doing it, would love some feedback on this if possible.

@Acly
Copy link
Owner

Acly commented Nov 21, 2024

It might be simpler, can you try:

# custom_workflow.py
class CustomWorkspace:
    def try_set_params(params: dict):
        self.params = _coerce(params, self.metadata)

# ui/generation.py in _copy_prompt
self._model.custom.try_set_params(job.params.metadata)

Should not require the signal, or any changes in the custom param UI part. And it will also take care of cases like having a different workflow which uses a param with the same name, but it has a different type (eg. string instead of number).

Edit: you might want a slightly different version of _coerce which uses the previous value if the job params don't have a match, rather than the default.

@ronan36880 ronan36880 force-pushed the feat/copy_prompt_parameters branch 3 times, most recently from 6c3785d to 2dff431 Compare November 22, 2024 11:54
@ronan36880
Copy link
Contributor Author

I tried going with that approach, I still had to emit the graph_changed signal for the UI widgets to update. Should the extra coerce logic be in a separate method like it is right now, or should they both be merged into one somehow?

@Acly
Copy link
Owner

Acly commented Nov 22, 2024

Should the extra coerce logic be in a separate method like it is right now, or should they both be merged into one somehow?

Isn't coerce the same as coerce_with_fallback and passing a empty dict {} as fallback?

@ronan36880 ronan36880 force-pushed the feat/copy_prompt_parameters branch from 2dff431 to c858f61 Compare November 22, 2024 19:16
@ronan36880
Copy link
Contributor Author

Should the extra coerce logic be in a separate method like it is right now, or should they both be merged into one somehow?

Isn't coerce the same as coerce_with_fallback and passing a empty dict {} as fallback?

Great catch! I went ahead and made the original _coerce call _coerce_with_fallback with an empty dict.

@Acly Acly merged commit e1c8d06 into Acly:main Nov 25, 2024
2 checks passed
@Acly
Copy link
Owner

Acly commented Nov 25, 2024

Merged, thanks for the PR!

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