-
Notifications
You must be signed in to change notification settings - Fork 2.5k
feat(ui): model relationship management #7963
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
base: main
Are you sure you want to change the base?
feat(ui): model relationship management #7963
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great, thank you! I've added some minor comments.
There will be some conflicts while rebasing bc I've updated the new model picker again. It will be a bit trickier to add the related models toggle to it.
Fortunately, because we are not allowing users to relate two main models, it's OK to omit the related toggle. In the future when we extend the new picker to other model types (e.g. LoRAs), we will figure out how to integrate related models.
For now, please drop the changes to the new model picker.
You can delete ParamMainModelSelect.tsx
, this file is no longer needed.
PS: I apologize for the delay in getting to review this!
invokeai/app/services/model_relationship_records/model_relationship_records_base.py
Outdated
Show resolved
Hide resolved
// so this mapping provides a slightly more expressive color flow. | ||
// | ||
// Note: This is purely aesthetic. Safe to remove if project preferences change. | ||
const getModelTagColor = (type: string): string => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets move this and getTypeFromLabel()
to be defined outside the component, as they do not depend on the component's state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved colors, type from label, fixed some bad comments, and defined the base set of Disallowed Relationships outside.
Also dried out the sorting method while I was at it.
Part of 3dc2e71
</FormControl> | ||
<Box> | ||
<Flex gap="2" flexWrap="wrap"> | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block of logic in the view code is hard to read - best to move this logic outside the JSX.
Here's an idea to make it a bit more conventional for React (make components smaller and more composable, with as little logic in the JSX as possible):
- Build a
relatedModelConfigs: AnyModelConfig[]
object that contains the model configs so we can skip the null checks - Iterate over the configs to group them by type, could end up with data like this:
type ModelGroup = { type: ModelType; modelConfigs: AnyModelConfig[] }; const groupedModelConfigs: ModelGroup[] = relatedModelKeys.reduce(...) const sortedGroupedModelConfigs = groupedModelConfigs.sort(...)
- Create a
<ModelTagGroup />
component that renders aModelGroup
- Handler for removing
const onRemove = useCallback((key: string) => handleRemove(key), [handleRemove])
- In the JSX:
{sortedGroupedModelConfigs.map((group) => <ModelTagGroup key={group.type} group={group} onRemove={onRemove} />}
- Create a
<ModelTag />
component that renders an individual model config as a tag.<ModelTagGroup />
would map over itsgroup.modelConfigs
returning this component.<ModelTag modelConfig={modelConfig} onRemove={onRemove} />
- For dividers, in the mapped rendering of the groups, you can do something like this:
{sortedGroupedModelConfigs.map((group, i) => { const withDivider = i < sortedGroupedModelConfigs.length - 1; return <React.Fragment key={group.type}> <ModelTagGroup group={group} onRemove={onRemove} /> {withDivider && <Divider />} </React.Fragment> } }
<TagLabel | ||
style={{ | ||
maxWidth: '50px', | ||
overflow: 'hidden', | ||
textOverflow: 'ellipsis', | ||
whiteSpace: 'nowrap', | ||
}} | ||
> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A pain point w/ React is that passing an inline object like this will re-create the object on every render cycle.
The component that receives the object prop does a shallow equality check to see if its incoming data has changed. So in this case, on every render cycle, <TagLabel />
will think it's getting a different set of props and re-render itself. Even tho the dict is the same
In this specific case, it's not a big problem. But if <TagLabel/>
was a heavy component, we would be wasting a lot of CPU cycles.
We can use inline style props to get around this like you've done elsewhere
<TagLabel | |
style={{ | |
maxWidth: '50px', | |
overflow: 'hidden', | |
textOverflow: 'ellipsis', | |
whiteSpace: 'nowrap', | |
}} | |
> | |
<TagLabel maxWidth="50px" overflow="hidden" textOverflow="ellipsis" whiteSpace="nowrap"> |
It's unfortunate that we need to worry about this stuff but that's how React works right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented in 3dc2e71.
Note: Since this was implemented outside the GitHub suggestion flow and followed by a force push, the UI no longer marks(or allows me to mark) the comment as accepted. The change is definitely present — just wanted to clarify that the suggestion wasn’t missed or ignored.
9631e4a
to
f8c63c1
Compare
Adds full support for managing model-to-model relationships in the UI and backend. Introduces RelatedModels subpanel for linking and unlinking models in model management. - Adds REST API routes for adding, removing, and retrieving model relationships. - New database migration: creates model_relationships table for bidirectional links. - New service layer (model_relationships) for relationship management. - Updated frontend: Related models float to top of LoRA/Main grouped model comboboxes for quick access. - Added 'Show Only Related' toggle badge to MainModelPicker filter bar **Amended commit to remove changes to ParamMainModelSelect.tsx and MainModelPicker.tsx to avoid conflict with upstream deletion/ rewrite**
removed unused AnyModelConfig related methods, removed unused get_related_model_key_count method.
Major cleanup of RelatedModels.tsx for improved readability, structure, and maintainability. Dried out repetitive logic Consolidated model type sorting into reusable helpers Added disallowed model type relationships to prevent broken connections (e.g. VAE ↔ LoRA) - Aware this introduces a new constraint—open to feedback (see PR comment) Some naming and types may still need refinement; happy to revisit
f8c63c1
to
3dc2e71
Compare
For the sake of a clean commit history, I reset and amended the PR commit to remove changes to ParamMainModelSelect.tsx and MainModelPicker.tsx. These files were either deleted or reworked upstream, so this avoids unnecessary conflicts. I opted not to cherry-pick the deletion or do rebase acrobatics until the PR is approved, to keep things easier to track and adjust if needed. Note: Some previous comments may now refer to code that no longer exists in the diff—feel free to flag anything still relevant, and I’ll handle it. |
Adds full support for managing model-to-model relationships in the UI and backend.
Introduces RelatedModels subpanel for linking and unlinking models in model management.
Summary
Introduces a full system for managing model-to-model relationships.
New database migration: model_relationships table (bidirectional, FK constrained, cascade deletes, sorted input).
New service layer: model_relationships for API interaction.
New UI subpanel (RelatedModels) for linking/unlinking models in the Model Manager.
UI on frontend linked to relationships, pulling either to the top, or allowing filters.
Rebased due to snazzy new model selector that happened this week.
Notes
base_model
types (or any model with the base typeany
) in the UI.related_model_ids
) on models for optimized frontend access (instead of extra API calls).First PR, Please be gentle.
Related Issues / Discussions
Part of 7894 used_with tag support.
QA Instructions
Ran Lint, Prettier, pnpm test, Made sure UI and API routes functioned with DB open to watch.
Merge Plan
Unless a migration happens between now and PR, Rebase if needed and repush? Honestly not completely sure what's needed here, but more than willing to work for whatever is needed.
Checklist
What's New
copy (if doing a release after this PR)