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

Refactor JS Client #7646

Merged
merged 95 commits into from
Apr 19, 2024
Merged

Refactor JS Client #7646

merged 95 commits into from
Apr 19, 2024

Conversation

hannahblair
Copy link
Collaborator

@hannahblair hannahblair commented Mar 8, 2024

Refactoring our Client from a giant function to a cleaner, more readable, fully typed Class constructor has meant that we need to change the way we initialise an instance of the client.

To create an instance of the client, we write the following:

const app = await Client.create("hmb/whisper")

Description

  • Convert Client.ts to a a Class
  • Add typing and remove all @ts-nochecks
  • Add unit tests for helpers and utils
  • Add integration tests: spin up gradio demo (as is done in Python Client) (to be done in a separate PR, msw needs to be setup and this PR is already quite big)
  • Update client/js/README and website docs with new usage pattern (Update Client usage examples #8003)
  • Add sse_v2.1 and sse_v3 to protocol list
  • Validate file upload (as is being done in the Python Client)

Testing

  • Core gradio functionality
  • Gradio/lite functionality
  • Use of @gradio/client as an external dependency
    • view_api,
    • predict (with files and primitive types) (with and without endpoint param)
    • submit, (on, off, cancel, destroy), `
    • duplicate

Closes:

🎯 PRs Should Target Issues

Before your create a PR, please check to see if there is an existing issue for this change. If not, please create an issue before you create this PR, unless the fix is very small.

Not adhering to this guideline will result in the PR being closed.

Tests

  1. PRs will only be merged if tests pass on CI. To run the tests locally, please set up your Gradio environment locally and run the tests: bash scripts/run_all_tests.sh

  2. You may need to run the linters: bash scripts/format_backend.sh and bash scripts/format_frontend.sh


🗑️ Outdated Description 🗑️

The reason why we can't simply initialise an app via a Class Constructor is because we need to fetch certain values (config, api, api_map etc) via asynchronous functions when an instance of the client is created, and a TS constructor cannot be async (perhaps without a janky workaround). So we need to decide on a new usage pattern.

We've got a few options:

  1. Readiness pattern
const app = new Client("url")
await app.ready;
app.on('ready', () => {...})

Add ready to the client; a Promise that resolves when the client is fully initialised. Implement an event listener for a ready event on the client. When the ready event is emitted by the client instance, the callback function provided will be executed

Nice article here

  1. init() function
const app = new Client()
await app.init()

Add an init() function (with the async initialisation logic) which the user manually calls and awaits

  1. Async initialiser function (implemented in this PR)
const app = await new Client.create("url")

Essentially uses a function like init() in option 2 but encapsulates this in a create() function so it can be used in one line

Initial thoughts:

@pngwn

  • app.on('ready', () => {...}) is a common pattern, technically most "correct", it also echoes other parts of the API, event driven, with the caveat of the api syntax
  • Client.create() is pretty different too tho, no more explicitly creating an instance of the class
  • prefer for python + js clients to be as consistent as possible

@hannahblair

  • the usage pattern for option 1 may too much of a change to whats in place now vs await new Client() -> await new Client.create()

Note: We already have the initialisation logic in place in the Client, and going ahead with any of the other two options won't mean too much work; just moving some code around a bit. This is just a matter of what pattern is most ideal.

@gradio-pr-bot
Copy link
Contributor

gradio-pr-bot commented Mar 8, 2024

🪼 branch checks and previews

Name Status URL
Spaces ready! Spaces preview
Website ready! Website preview
Storybook ready! Storybook preview
🦄 Changes detected! Details

Install Gradio from this PR

pip install https://gradio-builds.s3.amazonaws.com/c094ed7ef4f2f58c516d6fdd2182ed48141de17a/gradio-4.27.0-py3-none-any.whl

Install Gradio Python Client from this PR

pip install "gradio-client @ git+https://github.com/gradio-app/gradio@c094ed7ef4f2f58c516d6fdd2182ed48141de17a#subdirectory=client/python"

@gradio-pr-bot
Copy link
Contributor

gradio-pr-bot commented Mar 8, 2024

🦄 change detected

This Pull Request includes changes to the following packages.

Package Version
@gradio/app minor
@gradio/client minor
@gradio/spaces-test minor
gradio minor
  • Maintainers can select this checkbox to manually select packages to update.

With the following changelog entry.

Refactor JS Client

Maintainers or the PR author can modify the PR title to modify this entry.

Something isn't right?

  • Maintainers can change the version label to modify the version bump.
  • If the bot has failed to detect any changes, or if this pull request needs to update multiple packages to different versions or requires a more comprehensive changelog entry, maintainers can update the changelog file directly.

@pngwn
Copy link
Member

pngwn commented Mar 14, 2024

@hannahblair We chatted about this in retro and we agreed that option 3 was probably preferable:

const app = await Client.create();

The reasoning was as follows:

  • one liner gives nice ergonomics
  • Similar to the python API (although not identical)
  • Keeping the class as the external interface makes it easier to extend and provide custom fetch and websocket methods.
  • has a nice symmetry with Client.duplicate() which also returns and instance of the class.

I think we are good to finish this off in that direction.

@abidlabs
Copy link
Member

@hannahblair could we update the docs here: https://www.gradio.app/guides/getting-started-with-the-js-client as well?

@hannahblair
Copy link
Collaborator Author

@hannahblair could we update the docs here: https://www.gradio.app/guides/getting-started-with-the-js-client as well?

I've got a separate PR with docs changes in #8003. It's actually branching off this PR so if that's approved (and I've fixed those conflicts) I can merge it into this one.

@abidlabs
Copy link
Member

abidlabs commented Apr 18, 2024

Awesome PR @hannahblair. I tested a lot of Gradio, only 2 things I noticed:

(1) The image_editor_events demo was not working for me locally or on Spaces. If I make any change to the input, I see this error on the Space:

image

Locally, I see this error printed in the console:

KeyError: '7gksku7prxg'

Seems like maybe there's a conflict between recent changes to the ImageEditor and this client refactor PR.

(2) This is not super urgent, but if you look at the view API page for any endpoint that involves uploading a file, the file dictionary in the payload is very complex, e.g.:

image

This is the dict for a single file:

{"path":"https://github.com/gradio-app/gradio/raw/main/test/test_files/sample_file.pdf","meta":{"_type":"gradio.FileData"},"orig_name":"sample_file.pdf","url":"https://github.com/gradio-app/gradio/raw/main/test/test_files/sample_file.pdf"}

This is technically correct, but it would be nice to wrap this in a helper function so users don't have to think about all of the fields. For the Python Client, we have a file() helper function that just takes in the path to the file and populates the rest of the fields accordingly, maybe we could do the same for the js client.

I'm about to test some Gradio-Lite demos, will let you know if I see anything else. But so far, everything else works perfectly!

@abidlabs
Copy link
Member

Tested a few Gradio/Lite demos and they are working well from what I can tell!

hannahblair and others added 8 commits April 19, 2024 00:04
* update client examples

* remove test data

* fix types

* remove types changes

* client -> Client.create

* Update client/js/README.md

Co-authored-by: Abubakar Abid <abubakar@huggingface.co>

* Update client/js/README.md

Co-authored-by: Abubakar Abid <abubakar@huggingface.co>

* update duplicate docs

* attempt to update cn docs

* format

---------

Co-authored-by: Abubakar Abid <abubakar@huggingface.co>
@hannahblair
Copy link
Collaborator Author

hannahblair commented Apr 19, 2024

thanks for testing @abidlabs! fixed the image editor issue. good catch.

agree with your point about the view api docs - i'll fix this in a separate PR if that's all good

Copy link
Member

@abidlabs abidlabs left a comment

Choose a reason for hiding this comment

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

LGTM @hannahblair! This PR's good stuff

agree with your point about the view api docs - i'll fix this in a separate PR if that's all good

👍 I'll create an issue so we catch it before the 1.0 release

Copy link
Member

@pngwn pngwn left a comment

Choose a reason for hiding this comment

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

Awesome stuff! Good to merge imo. Thanks for taking this one on @hannahblair!

@hannahblair hannahblair merged commit 450b8cc into main Apr 19, 2024
8 checks passed
@hannahblair hannahblair deleted the refactor-client branch April 19, 2024 21:36
This was referenced Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants