-
Notifications
You must be signed in to change notification settings - Fork 2.7k
images refactor #3443
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
images refactor #3443
Conversation
8d514ab
to
ecd04da
Compare
I'd like to get a couple reviews of the overall design and changes to python code. The client side is still a WIP but the backend changes are almost fully implemented. Marking this ready for review in hopes of getting a review or two before I finish polishing the UI, in case there is some major issue I've missed. Besides the usual code review, this change needs careful thought. Once it rolls out to OSS and people are using it, we can't easily change db schemas. So long as there's no problem there, the rest is easy to fix. It's possible to run this PR now, same way you'd run If you run this locally, you can only use the node editor, and only with certain nodes. This is bc I haven't changed all the image-outputting nodes to use the new service yet. Suggest testing with exactly this graph layout for single images: Or this one for multiples: (you can change all parameters of course) |
feat(nodes): address feedback
|
5d55340
to
5668711
Compare
feat(nodes): add ResultsServiceABC & SqliteResultsService **Doesn't actually work bc of circular imports. Can't even test it.** - add a base class for ResultsService and SQLite implementation - use `graph_execution_manager` `on_changed` callback to keep `results` table in sync fix(nodes): fix results service bugs chore(ui): regen api fix(ui): fix type guards feat(nodes): add `result_type` to results table, fix types fix(nodes): do not shadow `list` builtin feat(nodes): add results router It doesn't work due to circular imports still fix(nodes): Result class should use outputs classes, not fields feat(ui): crude results router fix(ui): send to canvas in currentimagebuttons not working feat(nodes): add core metadata builder feat(nodes): add design doc feat(nodes): wip latents db stuff feat(nodes): images_db_service and resources router feat(nodes): wip images db & router feat(nodes): update image related names feat(nodes): update urlservice feat(nodes): add high-level images service
`set` is a python builtin
- Address database feedback: - Remove all the extraneous tables. Only an `images` table now: - `image_type` and `image_category` are unrestricted strings. When creating images, the provided values are checked to ensure they are a valid type and category. - Add `updated_at` and `deleted_at` columns. `deleted_at` is currently unused. - Use SQLite's built-in timestamp features to populate these. Add a trigger to update `updated_at` when the row is updated. Currently no way to update a row. - Rename the `id` column in `images` to `image_name` - Rename `ImageCategory.IMAGE` to `ImageCategory.GENERAL` - Move all exceptions outside their base classes to make them more portable. - Add `width` and `height` columns to the database. These store the actual dimensions of the image file, whereas the metadata's `width` and `height` refer to the respective generation parameters and are nullable. - Make `deserialize_image_record` take a `dict` instead of `sqlite3.Row` - Improve comments throughout - Tidy up unused code/files and some minor organisation
When returning a `FileResponse`, we must provide a valid path, else an exception is raised outside the route handler. Add the `validate_path` method back to the service so we can validate paths before returning the file. I don't like this but apparently this is just how `starlette` and `fastapi` works with `FileResponse`.
`stop` must be greater than `start`.
If `seed>SEED_MAX`, we can still continue if we parse the seed as `seed % SEED_MAX`.
The `RangeInvocation` is a simple wrapper around `range()`, but you must provide `stop > start`. `RangeOfSizeInvocation` replaces the `stop` parameter with `size`, so that you can just provide the `start` and `step` and get a range of `size` length.
* except i haven't rebuilt inpaint in latents
also make img node names consistent
this will be a model config feature when model manager is ready
23f6a82
to
f5539b4
Compare
I will add tests for the new service soon
@Kyle0654 @lstein - As mentioned in Discord, merging this in as this is needed to continue forward progress, but please feel free to provide feedback/thoughts to @psychedelicious where relevant |
General design changes
ImageCategory
model: images are designated asimage
(normal),control_image
(processed controlnet image), orother
(future use by community nodes). Any number of new categories can be added, these are just the ones I know we need now.Refactor images service:
34 low-level services and 1 high-levelImageFileStorage
: low-level service, responsible for image files, provided implementation uses disk storageImageRecordStorage
: low-level service, responsible for image records (ie the image tables in the db), provided implementation via the sqlite dbUrlService
: low-level service, responsible for generating URLs for images, provided implementation just formats path stringsMetadataService
: low-level service, responsible for generating metadata, provided implementation builds metadata the graph-traversal strategy described belowImagesService
: high-level service, exclusively provides app access to the low-level servicesAdd images-related tables to db
image_records_storage.py
for implementationimage_types
(existingImageType
model) andimage_categories
(newImageCategory
model) store possible values for their respective models. If the model is updated in the future, new values are added to the tables on startup.Metadata
t2l
orl2l
node and thenoise
andcompel
predecessor nodes from that. Metadata to be included are listed belowTODO
Refactor web API routes:
TODO
Get image urls added toinstead, we make an extra round trip to retrieve an image and its metadata. We need to make the additional request to get the full metadata anyways so it's not really a waste. This is implemented.invocation_complete
events (dirty hack in place now)Hook up the metadata building functions (they are implemented and functional but not used)Only thetxt2img
node is hooked upLatentsToImage
is connected, but all nodes that output an image need to be hooked upTODO (in a future PR)
latents
(which, more generally, are pickle-abletorch
objects). These includeimage_latents
andconditioning
right now, but I think we will see a lot more types of these coming up. Probably need to give these objects similar treatment to images.