gradio icon indicating copy to clipboard operation
gradio copied to clipboard

Refactor Interface

Open omerXfaruq opened this issue 2 years ago • 4 comments

  • [X] I have searched to see if a similar issue already exists.

I have some concerns about how Interface is designed right now, and I feel like we need to refactor it quite a bit. Previously it was our core class, however as Blocks got out, it is now just a wrapper around Blocks. But the current design is too complex, and making changes on it affects a lot of parts. Currently I added support for async functions within Blocks, however adding it into Interface is quite troublesome, since it creates a very big chain, requiring many functions to be async and quite a lot of change in the code base and tests. I think we should refactor Interface smoothly after 3.0, WDYT?

Summary IMO all functions in Interface should be moved to Blocks and Interface should just a type of Blocks, if possible.

Context fn being async affects -> self.predict image

self.predict -> run_prediction image run_prediction affects a few more functions like process and run_interpret and all needs to be async functions recursively

which creates a very big chain, requiring quite a lot of change in the code base and tests

omerXfaruq avatar May 11 '22 09:05 omerXfaruq

I don't think these should continue to exist within or related to Interface as now our core class is Blocks and not Interface.

image image image

omerXfaruq avatar May 11 '22 10:05 omerXfaruq

This is very cool!

I think the biggest impact will be in cleaning up the discrepancies between Interface.process, Interface.run_prediction, Interface.__call__ and Blocks.__call__. They are all doing similar things and it's hard to understand the differences (or which one should be used when) as a developer.

Agreed that the functions in interpretation.py and Interface.interpret should live in their own dedicated component similar to what we have done for examples and the Interface constructor uses that component if interpretation is required. (Tracked by #1831).

I don't think we should move load and from_pipeline from Interface to Blocks. They are not doing much (and already delegate to Blocks) and having them defined in Interface let's us have interface-specific docstrings and document them on the website.

freddyaboulton avatar Jul 19 '22 16:07 freddyaboulton

Totally agree, that's what I was thinking, we're currently looking to usage of interpretation through analytics with @abidlabs, and if the usage is very low, we might remove and redesign interpretation with Blocks as well.

omerXfaruq avatar Jul 19 '22 16:07 omerXfaruq

@abidlabs would you like to take this on? As it is difficult to clean unnecessary parts in Interface without knowing the previous design&configurations, I think this would suit you best.

omerXfaruq avatar Jul 21 '22 23:07 omerXfaruq

Update: Interface was updated in the shortest manner to update async functions. I also dropped some comments inside interface for refactoring. A proper refactoring for Blocks and Interface would make the code-base much more tidy and easy to follow. I think the difference between the Interface and Blocks should be minimized since Interface is just a child or specific mode of Blocks.

omerXfaruq avatar Aug 19 '22 20:08 omerXfaruq

Thanks for highlighting this issue @FarukOzderim. I worked on this as part of #1967, and eliminated both Interface.process() and Interface.__call__ as separate methods of the Interface. I think the PR basically covers this issue (and I'll update the PR description to reflect that). Let me know what you think!

abidlabs avatar Aug 21 '22 02:08 abidlabs

I took a look to the final version of Interface. It is great that is shortened, but I still feel that the Interface has a lot of code, considering it is just a specific version of Blocks, and still think that a proper refactor would be great.

  1. We could Interface much shorter with cleanups, and moving parts to Blocks.
  2. Make Interface modular with functions as it is very hard to follow or read it

I might be overdoing or overthinking this, hence a look to Interface from @freddyaboulton and @aliabid94 would be great. And I think we could discuss and finalize this with a small huddle/meeting.

omerXfaruq avatar Aug 22 '22 15:08 omerXfaruq

Btw this issue will close when the PR is merged, care!

omerXfaruq avatar Aug 22 '22 15:08 omerXfaruq

@FarukOzderim what further cleanups are you suggesting? I am rewriting Interface.run_prediction to use Block.call_function instead, but is there anything else that can be moved to Blocks?

abidlabs avatar Aug 22 '22 19:08 abidlabs

Oh yes I worked on this as part of PR #1967. Will update the PR description to reflect this

On Fri, Aug 19, 2022 at 3:42 PM Ömer Faruk Özdemir @.***> wrote:

Update: Interface was updated in the shortest manner to update async functions. I also dropped some comments inside interface for refactoring. A proper refactoring for Blocks and Interface would make the code-base much more tidy and easy to follow. I think the difference between the Interface and Blocks should be minimized since Interface is just a child or specific mode of Blocks.

— Reply to this email directly, view it on GitHub https://github.com/gradio-app/gradio/issues/1212#issuecomment-1221077896, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANSE6LY6BBSYZ42JVVNWKTVZ7WUPANCNFSM5VULEEYA . You are receiving this because you were mentioned.Message ID: @.***>

abidlabs avatar Oct 11 '22 09:10 abidlabs