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(dev_reload): use jurigged to hot reload changes to Python source #4313

Merged
merged 9 commits into from
Aug 25, 2023

Conversation

keturn
Copy link
Contributor

@keturn keturn commented Aug 18, 2023

While it is not necessary to change the application source to use jurigged for live reloading, it does help discoverability to have the option show up in our Config instead of relying on developers having that particular bit of Python know-how.

Programmatically starting jurigged like this also allows for better integration with our logging configuration, though the way I've integrated it here lacks the color-coding provided by jurigged's default.

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

  • (developer) Feature

Have you discussed this change with the InvokeAI team?

See livereload / hot reloading Python development server.

Have you updated all relevant documentation?

  • Yes

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

Start the server with the --dev_reload flag.

Change a Python source file and note the behavior changes without having to shut down and restart the server.

Added/updated tests?

  • No, I'm not writing tests that modify the Python sources to check how a running server responds.

@lstein
Copy link
Collaborator

lstein commented Aug 18, 2023

Very cool! If it's still unmerged I'll give it a thorough review when I'm back from camping in a couple of days.

@keturn keturn added enhancement New feature or request python PRs that change python files labels Aug 18, 2023
Copy link
Collaborator

@psychedelicious psychedelicious left a comment

Choose a reason for hiding this comment

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

Woohoo! It works wonderfully.

One caveat - when changing a node:

  • the InvocationsUnion doesn't change and Graph model isn't re-initialized
  • so the OpenAPI schema is not updated
  • so changes to anything other than the body of each node's invoke() function are not hot-reloaded

For example, changing the name, type, or fields of a node does not change trigger a reload. This is expected, but it would be very nice if that could all reload too.

I made some progress on this in #4334. Can we provide callbacks to jurrigged to execute on certain events? Specifically, can we make any hot reload in invocations/*.py trigger logic similar to #4334?

@keturn
Copy link
Contributor Author

keturn commented Aug 22, 2023

Can we provide callbacks to jurrigged to execute on certain events?

Hmm, looks like yes, but let's leave that to a later PR so we can explore that whole mess of Invocation loading and introspection separately.

@damian0815
Copy link
Contributor

sooo on macos, ootb jurigged is broken because of some change to whichever flavour of FSEvents system that the fruit company wants us to use in 2023. is this able to act as a workaround?

@keturn
Copy link
Contributor Author

keturn commented Aug 23, 2023

sooo on macos, ootb jurigged is broken [...] is this able to act as a workaround?

This is literally just instantiating a jurigged watcher, so I expect it inherits any jurigged brokenness.

@damian0815
Copy link
Contributor

ahh. oh well, thanks.

Copy link
Collaborator

@lstein lstein left a comment

Choose a reason for hiding this comment

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

LGTM

@keturn keturn merged commit 6b462f2 into main Aug 25, 2023
8 checks passed
@keturn keturn deleted the feat/dev_reload branch August 25, 2023 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request python PRs that change python files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants