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.

Sign up for GitHub

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

Jump to bottom

refactor(events): use pydantic schemas for events #5748

Merged
merged 43 commits into from
May 26, 2024
Merged

refactor(events): use pydantic schemas for events #5748

merged 43 commits into from
May 26, 2024

Conversation

psychedelicious
Copy link
Collaborator

@psychedelicious psychedelicious commented Feb 19, 2024 •
edited

What type of PR is this? (check all applicable)

  • Refactor

Have you discussed this change with the InvokeAI team?

  • Yes
  • No, because: Nerd sniped myself

Description

refactor(events): use pydantic schemas for events

Our events handling and implementation has a couple pain points:

  • Adding or removing data from event payloads requires changes wherever the events are dispatched from.
  • We have no type safety for events and need to rely on string matching and dict access when interacting with events.
  • Frontend types for socket events must be manually typed. This has caused several bugs.

fastapi-events has a neat feature where you can create a pydantic model as an event payload, give it an __event_name__ attr, and then dispatch the model directly.

This allows us to eliminate a layer of indirection and some unpleasant complexity:

  • Event handler callbacks get type hints for their event payloads, and can use isinstance on them if needed.
  • Event payload construction is now the responsibility of the event itself (a pydantic model), not the service. Every event model has a build class method, encapsulating this logic. The build methods are provided as few args as possible. For example, InvocationStartedEvent.build() gets the invocation instance and queue item, and can choose the data it wants to include in the event payload.
  • Frontend event types may be autogenerated from the OpenAPI schema. We use the payload registry feature of fastapi-events to collect all payload models into one place, making it trivial to keep our schema and frontend types in sync.

Note: Over half the lines in this PR are from the new autogenerated types. This PR is otherwise a net reduction in LOC.

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

Give the app a spin. Things that use events:

  • Graph execution (session- and invocation-scoped events)
  • Enqueuing
  • Clearing the queue
  • Model loading
  • Model download and install
  • Bulk downloads

Merge Plan

This PR can be merged when approved

Added/updated tests?

  • Yes
  • No

@github-actions github-actions bot added api python PRs that change python files services PRs that change app services frontend PRs that change frontend files labels Feb 19, 2024
@psychedelicious psychedelicious marked this pull request as draft February 19, 2024 11:16
Base automatically changed from refactor/nodes/merge-processors to next February 19, 2024 22:54
@psychedelicious psychedelicious force-pushed the psyche/refactor/events branch 3 times, most recently from dd8c4a4 to c496cc0 Compare February 21, 2024 09:36
@github-actions github-actions bot added documentation Improvements or additions to documentation invocations PRs that change invocations backend PRs that change backend files installer PRs that change the installer CICD FrontendDeps labels Mar 10, 2024
@psychedelicious psychedelicious changed the base branch from next to main March 10, 2024 08:55
@github-actions github-actions bot added the python-tests PRs that change python tests label Mar 10, 2024
@psychedelicious
feat(events): remove payload registry, add method to get event classes
0158b22 
We don't need to use the payload schema registry. All our events are dispatched as pydantic models, which are already validated on instantiation.

We do want to add all events to the OpenAPI schema, and we referred to the payload schema registry for this. To get all events, add a simple helper to EventBase. This is functionally identical to using the schema registry.
@psychedelicious
chore(ui): typegen
d994dfa
@psychedelicious
fix(events): fix denoise progress percentage
cad6050 
- Restore calculation of step percentage but in the backend instead of client
- Simplify signatures for denoise progress event callbacks
- Clean up `step_callback.py` (types, do not recreate constant matrix on every step, formatting)
@psychedelicious
feat(api): sort socket event names for openapi schema
34612c4 
Deterministic ordering prevents extraneous, non-functional changes to the autogenerated types
@psychedelicious
chore(ui): typegen
8ed3923
@psychedelicious
fix(ui): denoise percentage
e4abdda
@psychedelicious
docs: clarify comment in api_app
b9ece91
@psychedelicious
tidy(ui): remove old unused session subscribe actions
20299df
@psychedelicious
chore(ui): typegen
81cb1ff
@psychedelicious
fix(ui): update event handling to match new types
8bfc443
@psychedelicious
fix(mm): port changes into new model_install_common file
806c4a9 
Some subtle changes happened between this PR's last update and now. Bring them into the file.
@psychedelicious
tests: clean up tests after events changes
abd6418
@psychedelicious
feat(events): restore temp handling of user/project
0606e8f
@psychedelicious
chore(ui): typegen
f9a9301
@psychedelicious
tidy(processor): use separate handlers for each event type
69cfbb6 
Just a bit clearer without needing `isinstance` checks.
@psychedelicious
fix(processor): move session started event to session runner
83092b4
@psychedelicious
feat(ui): add logging for session events
3a0586c
@psychedelicious
fix(mm): emit correct event when model load complete
7979356
@psychedelicious
tidy(events): use tuple index access for event payloads
3340b0d
@psychedelicious
feat(events): register_events supports single event
44c6535
@psychedelicious
feat(events): move event sets outside sio class
55bf194 
This lets the event sets be consumed programmatically.
@psychedelicious
refactor: remove all session events
f96d2b9 
There's no longer any need for session-scoped events now that we have the session queue. Session started/completed/canceled map 1-to-1 to queue item status events, but queue item status events also have an event for failed state.

We can simplify queue and processor handling substantially by removing session events and instead using queue item events.

- Remove the session-scoped events entirely.
- Remove all event handling from session queue. The processor still needs to respond to some events from the queue: `QueueClearedEvent`, `BatchEnqueuedEvent` and `QueueItemStatusChangedEvent`.
- Pass an `is_canceled` callback to the invocation context instead of the cancel event
- Update processor logic to ensure the local instance of the current queue item is synced with the instance in the database. This prevents race conditions and ensures lifecycle callback do not get stale callbacks.
- Update docstrings and comments
- Add `complete_queue_item` method to session queue service as an explicit way to mark a queue item as successfully completed. Previously, the queue listened for session complete events to do this.

Closes #6442
@psychedelicious
chore(ui): typegen
bc274a8
@psychedelicious
feat(ui): update client for removal of session events
c5738c4
@psychedelicious
feat(ui): simplify client sio redux actions
f5e9707 
- Add a simple helper to create socket actions in a less error-prone way
- Organize and tidy sio files
@psychedelicious
docs: update comments & docstrings
3330337
@psychedelicious
docs: update docstrings in sockets.py
9e99cc8
@psychedelicious psychedelicious merged commit 8498d43 into main May 26, 2024
14 checks passed
@psychedelicious psychedelicious deleted the psyche/refactor/events branch May 26, 2024 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maryhipp maryhipp maryhipp approved these changes

@hipsterusername hipsterusername hipsterusername approved these changes

@blessedcoolant blessedcoolant Awaiting requested review from blessedcoolant blessedcoolant is a code owner

@brandonrising brandonrising Awaiting requested review from brandonrising brandonrising is a code owner

Assignees
No one assigned
Labels
api backend PRs that change backend files CICD documentation Improvements or additions to documentation frontend PRs that change frontend files FrontendDeps installer PRs that change the installer invocations PRs that change invocations python PRs that change python files python-deps PRs that change python dependencies python-tests PRs that change python tests PythonDeps PythonTests Root services PRs that change app services
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[enhancement]: Simplify session events [enhancement]: Use pydantic models for events
3 participants
@psychedelicious @maryhipp @hipsterusername