Add models as first supported 'asset' type #10045
Conversation
…ts for input validation
…ate endpoint for test suite.
[Assets] Initial implementation
…g yet uses the hashes
|
just fyi: CI fails on macOS due to missing |
) ## Summary Moves the fetch and post-fetch logic associated with the asset browser into the component and shows a loading state while fetching. To test, use this branch: comfyanonymous/ComfyUI#10045 https://github.com/user-attachments/assets/718974d5-efc7-46a0-bcd6-e82596d4c389 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-6189-load-assets-browser-before-fetch-completes-and-show-loading-state-2946d73d365081879d1bd05d86e8c036) by [Unito](https://www.unito.io) --------- Co-authored-by: GitHub Action <[email protected]>
) ## Summary Moves the fetch and post-fetch logic associated with the asset browser into the component and shows a loading state while fetching. To test, use this branch: comfyanonymous/ComfyUI#10045 https://github.com/user-attachments/assets/718974d5-efc7-46a0-bcd6-e82596d4c389 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-6189-load-assets-browser-before-fetch-completes-and-show-loading-state-2946d73d365081879d1bd05d86e8c036) by [Unito](https://www.unito.io) --------- Co-authored-by: GitHub Action <[email protected]>
There was a problem hiding this comment.
I reviewed the PR with comfy today. The biggest issues are that there are parts of code that seem to have unnecessary mistakes, and there are still references in code for postgres (I know some were removed previously already). Because the size of this PR is so big and enables things that aren't being used yet, the above issues are making him question the quality of what we are trying to get in.
I think if we want to get this through, we need to lock in and only keep things that are relevant to what we are doing right now. Otherwise, there is going to be the feeling for comfy that this is a sort of 'trojan horse' of a PR that has a bunch of unnecessary things in it.
We don't need this PR to do everything, we don't even know the scope of the assets on local besides models tbh. I can provide more guidance on slack.
| if field is None: | ||
| break | ||
|
|
||
| fname = getattr(field, "name", "") or "" |
There was a problem hiding this comment.
This was something comfy spotted, where the or "" is unnecessary since getattr already returns "", and there are some similar things like with the os.path.exists(tmp_path or "") check lower down in this function.
It is giving comfy a bad code smell; it makes him worried that if this part of the code has questionable things like this, then what if there are other parts of the code he has not looked at or has not as deep an understanding of that have the same issues.
There was a problem hiding this comment.
I can remove that, but then there should be check for None value.
This string guarantees that the fname will be string type.
second part os.path.exists(tmp_path or "") - is a typo
|
|
||
|
|
||
| def _hash_file_obj_sync(file_obj: IO[bytes], chunk_size: int) -> str: | ||
| """Hash an already-open binary file object by streaming in chunks. |
There was a problem hiding this comment.
Comfy is very confused why this file was written like this, given this PR does not actually add any hashing capabilities. For now we can just have a simple 'please hash this file' function. Hashing is not actually enabled in this PR, but we should clean it up. I know it's probably just remnants of the initial implementation you did with background hashing processes that enabled mid-hashing resume, but it's not we probably should not worry about future needs and focus on just the basics.
3 participants
With most of the networking and db code done by @bigcat88 , this PR:
Models are the only thing that will be treated as 'assets' currently. Goal is for the hash + tags to be used in a new Asset widget that will allow workflows to embed the hash of models, so that it will be easy to share workflows and know exactly what model is needed.
As the first step, while this PR does introduce blake3 hashing, it does not actually make use of it since the frontend elements are not ready - so no hashing enabled just yet. When enabled in the future, it will hash just the first time a model is loaded.
ETA: will review with @comfyanonymous on Saturday 9/27 to determine if any changes need to be made to this initial version.