(shortfin-apps)(flux) Align flux response handling with shark-ui compatibility requirements #1128
Conversation
monorimet
force-pushed
the
flux-response
branch
from
8c7f4c8 to
0eb3b2d
Compare
There was a problem hiding this comment.
l'm glad that this was easy enough to find, @monorimet! I believe there's an opportunity to keep flux and sd in-step with each other.
What if, instead of duplicating, we moved the existing copy from shortfin_apps/sd/components to a new shortfin_apps/text_to_image directory to keep the common, model-agnostic components separate? In the future, if needed, we can just subclass this for aspects specific to SDXL or Flux.
What do you think?
There was a problem hiding this comment.
I'd like to defer this to the coming refactor patches.
There was a problem hiding this comment.
We can defer if you'd like, totally!
But I recommend doing this now mainly because:
- It shrinks the diff for this particular PR (renaming an existing file with no line changes rather than adding a few dozen lines in the form of a new file)
- It's a simple, easy thing we can do right now that will shorten the list of things to refactor.
- Which as a result can simplify or completely avoid the merge conflicts we might encounter along the way.
Would you still like to defer?
There was a problem hiding this comment.
What about using the existing shortfin_apps/types directory?
There was a problem hiding this comment.
Great question!
The intent for that directory was more like "here's the stuff used exclusively by shortfin apps, but it's actually shortfin-agnostic", so probably not shortfin_apps/types. Now that you mention it, we'll probably wanna rename it to shortfin_apps/library the next time we mess with that directory. Not in this PR of course.
TextToImageInferenceOutput is definitely "aware" of shortfin's primary objective, and the name suggests that we'll eventually define TextToImageInferenceInput, so reserving shortfin_apps/text_to_image would help us begin with the end in mind!
And then when TextToTextInferenceOutput/TextToTextInferenceInput enter the chat, we can update our directories accordingly haha.
|
|
||
| async def send_request(session, rep, args, data): | ||
| async def send_request( | ||
| session: aiohttp.ClientSession, rep: int, args: argparse.Namespace, data: dict |
There was a problem hiding this comment.
I think there's an opportunity for a separate 30sec PR right here! I also have some more thoughts on this, but don't wanna clog up this review haha.
Could you bubble this signature change to its own commit at the beginning of this branch so we can cut it into a stack?
There was a problem hiding this comment.
This seems unnecessary given the commits will be squashed before merge.
There was a problem hiding this comment.
On that basis, totally unnecessary, for sure! I'd say it's more about PR granularity, diff size, merge conflict avoidance, etc. We can skip bubbling up the commit for now!
The thing I was gonna mention is making the arguments multiline for easier diffing:
| session: aiohttp.ClientSession, rep: int, args: argparse.Namespace, data: dict | |
| session: aiohttp.ClientSession, | |
| rep: int, | |
| args: argparse.Namespace, | |
| data: dict, |
What do you think?
There was a problem hiding this comment.
Yeah that seems reasonable.
There was a problem hiding this comment.
The pre-commit actually undoes this change... let's keep aligned with the automatic linter in this case.
There was a problem hiding this comment.
Ah yes, just tried it, and it seems to only undo the change if the dangling comma is left off! Does it pass pre-commit for you if you include the comma after dict?
monorimet
force-pushed
the
flux-response
branch
from
0a278eb to
cfb80af
Compare
Mirrors changes from the following sd-focused PRs:
#1025
#1068
#1069