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): connection validation rework #6386

Merged
merged 55 commits into from
May 19, 2024

Conversation

psychedelicious
Copy link
Collaborator

Summary

Major Changes:

  • feat(ui): add originalType to FieldType, improved connection validation

    We now keep track of the original field type, derived from the python type annotation in addition to the override type provided by ui_type.

    This makes ui_type work more like it sound like it should work - change the UI input component only.

    Connection validation is extend to also check the original types. If there is any match between two fields' "final" or original types, we consider the connection valid.This change is backwards-compatible; there is no workflow migration needed.

  • feat(ui): add ModelIdentifierField field type

    This new field type accepts any model. A field renderer lets the user select any available model.

  • feat(nodes): add ModelIdentifierInvocation

    This node allows a user to select any model, outputting a ModelIdentifierField for that model.

Minor Changes:

Related Issues / Discussions

Closes #6380

QA Instructions

The implementation in this PR is maximally flexible.

The increased flexibility comes at the cost of greater potential for footgun. Because model fields accept connections, and we allow any model field as an input, users could accidentally connect invalid fields. For example, a LoRA model could be connected to a ControlNet model input.

We could reduce the flexibility in a few ways:

  • Revert the change to allow all model fields to accept input connections and/or revert the new ModelIdentifierInvocation.
  • Add a setting to enable the new connection validation logic. It would be disabled by default.

Merge Plan

n/a

Checklist

  • The PR has a short but descriptive title, suitable for a changelog
  • Tests added / updated (if applicable)
  • Documentation added / updated (if applicable)

@github-actions github-actions bot added python PRs that change python files invocations PRs that change invocations frontend PRs that change frontend files labels May 17, 2024
@psychedelicious
Copy link
Collaborator Author

@skunkworxdark
This change enables your nodes. I made some changes to the core nodes so that anything that has a model drop-down can accept an input, and added an input UI component for plain ModelIdentifierField. It renders a list of all models.

This does render some of the nodes in your XY pack obsolete, hopefully that's a good thing!

@psychedelicious psychedelicious force-pushed the psyche/feat/ui/nodes-original-types branch from 6bab1a1 to 2601ac0 Compare May 17, 2024 11:06
@skunkworxdark
Copy link
Contributor

@psychedelicious
Firstly, I'm fine about it making any of my nodes redundant. Anything that improves core I am happy with.

After a brief look at the changes, I have no issues with this PR.

I assume that these changes don't alter the behaviour of the existing nodes like MainModelLoaerInvocation beyond allowing setting via a ModelIdentifierInvocation. i.e. the SDXLModelLoaderInvocation will only continue to display only SDXL models in the UI dropdown and the LoRALoaderInvocation will only LoRAs.

So the potential footgun moment only comes if the new ModelIdentifierInvocation is used and the user misselects a model or connects it to the wrong place. If this is the case then the chances of getting it wrong are low as most people will probably continue to use the existing loaders to select models in the workflows.

@psychedelicious
Copy link
Collaborator Author

psychedelicious commented May 17, 2024

@skunkworxdark
Thanks for reviewing.

So the potential footgun moment only comes if the new ModelIdentifierInvocation is used and the user misselects a model or connects it to the wrong place. If this is the case then the chances of getting it wrong are low as most people will probably continue to use the existing loaders to select models in the workflows.

Yep, that's right. I'm nervous about breaking the seal of our relatively foolproof connection validation logic though. Maybe we can hint to the user that the connection they are making is risky - like highlighting the field with a warning color. I'll try some things out.

@psychedelicious psychedelicious force-pushed the psyche/feat/ui/nodes-original-types branch from 2601ac0 to be674a9 Compare May 18, 2024 07:38
@skunkworxdark
Copy link
Contributor

Yep, that's right. I'm nervous about breaking the seal of our relatively foolproof connection validation logic though. Maybe we can hint to the user that the connection they are making is risky - like highlighting the field with a warning color. I'll try some things out.

That does sound like a good way of visualising a potential problem that would normally be hidden from the user. So if a connection is accepted on the wider type then highlight it with a different colour or line style.

@psychedelicious psychedelicious marked this pull request as draft May 18, 2024 11:53
@psychedelicious
Copy link
Collaborator Author

psychedelicious commented May 19, 2024

As I was working through the edge cases, I was increasingly unsure of connection validation. I've rewritten all of it - with full test coverage. This teased out a number of unknown edge cases, now resolved.

image

The connection validation logic also much cleaner. For example, before we had ~3 different places where connection validation logic was implemented, each in a slightly different way. The logic is now consolidated to a handful of utilities (each fully tested).

There are is still some a jank around edge updating but the core is sooo much better.

@psychedelicious
Copy link
Collaborator Author

  • Resolved edge updating jank and some issues with edge styling.
  • Fixed an issue with linear workflow view.
  • Removed a lot of extraneous actions and complexity. We now rely much more on reactflow's state management. Bug surface area for nodes is much smaller. This also makes the undo/redo more consistent because there are fewer combinations of actions.

…tion

We now keep track of the original field type, derived from the python type annotation in addition to the override type provided by `ui_type`.

This makes `ui_type` work more like it sound like it should work - change the UI input component only.

Connection validation is extend to also check the original types. If there is any match between two fields' "final" or original types, we consider the connection valid.This change is backwards-compatible; there is no workflow migration needed.
This new field type accepts _any_ model. A field renderer lets the user select any available model.
This node allows a user to select _any_ model, outputting a `ModelIdentifierField` for that model.
@psychedelicious psychedelicious force-pushed the psyche/feat/ui/nodes-original-types branch from eef6bb0 to 1fefe3b Compare May 19, 2024 05:52
@psychedelicious psychedelicious marked this pull request as ready for review May 19, 2024 05:55
@psychedelicious psychedelicious changed the title feat(ui): add original field types to node templates, add model identifier node feat(ui): connection validation rework May 19, 2024
@psychedelicious psychedelicious force-pushed the psyche/feat/ui/nodes-original-types branch from ca49a4d to 31d43cc Compare May 19, 2024 09:45
@psychedelicious
Copy link
Collaborator Author

I tried to add a warning when falling back on the original type, but propagating a warning got hairy. For now, that node is marked as prototype and its description explains the dangers. Hopefully that's enough to prevent footguns - worst case scenario - you get an error, not a big deal.


I'm happy with where this is now. I did some careful testing and fixed a couple outstanding issues.

@psychedelicious psychedelicious merged commit ca186bc into main May 19, 2024
14 checks passed
@psychedelicious psychedelicious deleted the psyche/feat/ui/nodes-original-types branch May 19, 2024 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend PRs that change frontend files invocations PRs that change invocations python PRs that change python files
Projects
None yet
3 participants