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 api logic, create JS API client #3334

Closed
wants to merge 44 commits into from
Closed

Refactor api logic, create JS API client #3334

wants to merge 44 commits into from

Conversation

pngwn
Copy link
Member

@pngwn pngwn commented Feb 27, 2023

Description

Implements JS API client.

Closes #3310. Closes #3262. Closes #3263. Closes #3269.

This PR implements a JS client for the gradio API. The API looks like this:

import { client } from "@gradio/client";

const app = await client('space-name-user.hf.space', status_callback);
const prediction = app.predict(endpoint, payload);

let x;
// listen for predictions
prediction.on("data", (event: { data: Array<unknown>; type: "data" }) => {
 x = data
});

// listen for status updates
prediction.on("status", (event: { data: Status; type: "data" }) => {});

// stop listening
prediction.off("data");

// cancel a prediction if it is a generator
prediction.cancel();

// chainable
const prediction_two = app
  .predict(endpoint, payload)
  .on("data", data_callback)
  .on("status", status_callback);

This is pretty idiomatic in javascript but more importantly it allows for a very clean and consistent interface.

By utilising events instead of awaiting singular responses we can have the same interface regardless of whether we are dealing with a simple prediction function, a generator with a finite amount of iterations or a generator which runs until cancelled (every).

i did consider an API like:

import { client } from "@gradio/client";

const app = await client();
const prediction_result = await app.predict(endpoint, payload, status_callback);

But then we would have needed a separate method for generator functions with a very different API. The equivalent for generators would have been:

import { client } from "@gradio/client";

const app = await client();

const prediction_result = await app.predict( // what would await even mean here?
  endpoint, 
  payload, 
  generation_callback,
  status_callback
);

This number of 'key' arguments is really not very cool in JS, we could opt for an 'options' object instead but ideally options are optional, at least 3/4 of these options are required most of the time. The number of arguments also makes the payload a required argument, which isn't ideal. Passing null or undefined is again not really ideal and not very idiomatic. The API proposed at the top (and implemented in this PR) is cleaner, more consistent, more flexible, and does a better job of abstracting away the implementation of the functions.

predict.cancel

This is slightly different to how cancellation works in Gradio itself. Gradio has the concept of functions cancelling other functions but this doesn't make sense for the API. If we enabled functions to cancel other functions, a decision which is usually based on the input + output components, then UI logic around the predict functions would start to bleed into the API. Prediction functions are discreet operations that do not know anything about one another and are not tied to one another (we simply bind them together via the UI), this client API represents that. With that in mind, there is an explicit 'cancel' method which will cancel a generator function if it is one. It is a noop otherwise. Calling other endpoint while one is still running will not interfere with it in anyway.

Happy to discuss this but I think it is important that we treat the API as a true API. The person consuming the API maybe not be the original author of the Gradio app and may have different intentions, I think we should offer that flexibility.

TypeScript

The client is pretty well typed too, so you get really good type checking with different callback types for different event types. It was painful but I'm pretty pleased with the results.

Screen.Recording.2023-02-27.at.23.55.43.mov

@gradio-pr-bot
Copy link
Collaborator

gradio-pr-bot commented Feb 27, 2023

🎉 The demo notebooks match the run.py files! 🎉

@gradio-pr-bot
Copy link
Collaborator

All the demos for this PR have been deployed at https://huggingface.co/spaces/gradio-pr-deploys/pr-3334-all-demos

@pngwn pngwn changed the title 3263 api Refactor api logic, create JS API client Feb 27, 2023
@pngwn pngwn marked this pull request as ready for review February 28, 2023 15:23
@pngwn
Copy link
Member Author

pngwn commented Feb 28, 2023

To test this:

  • try some demos
  • try with the queue enabled and not enabled
  • try with queue enabled + disabled
  • try generator functions (finite and infinite)
  • try function cancellation
  • try streaming (which was my biggest issue but works now)
  • Test embeds with hf subdomains(we can publish a beta to make this easier)

I have tested everything except embeds, doing that now.

@pngwn
Copy link
Member Author

pngwn commented Feb 28, 2023

better pass now

@freddyaboulton
Copy link
Collaborator

Did some testing @pngwn !

I am leaving some notes below but most demos are working well for me!

  1. Iterative outputs are not shown and the queue size is wrong, says null. Cancelling seems to be working as expected though.

iterative_events_not_showing

  1. The dropdown change event is being triggered twice the number of expected times. I'm seeing this in blocks_update and blocks_multiple_event_triggers but nowhere else. With and without the queue.

double_queue

  1. I think gr.Error is not working anymore. I got this when dividing by 0 in the calculator demo

image

  1. The sine_curve demo has an infinite generator. The plot is not reacting to the value of the slider anymore. And when I first loaded the app, I had to wait a while before the plot even showed up. I maybe had to move the slider values a bit but I can't remember for sure. This looks similar to 1.

This branch

since_curve_no_update

Main

since_curve_update

@pngwn
Copy link
Member Author

pngwn commented Feb 28, 2023

Thanks @freddyaboulton! Will take a look at these issues.

@pngwn
Copy link
Member Author

pngwn commented Mar 1, 2023

The dropdown change event is being triggered twice the number of expected times. I'm seeing this in blocks_update and blocks_multiple_event_triggers but nowhere else. With and without the queue.

@freddyaboulton Are you sure they are the same event trigger? That apphas a situation where a.change updates b which trigger b.change. I think you are just seeing all of that happen instantly because you are local. Do you get the same behaviour if. you apply some throttling to you local network? Alternatively could you inspect the messages and check if their payloads are the same or different?

@freddyaboulton
Copy link
Collaborator

@pngwn Doesn't happen locally on main 🤔

no_double_event_main

@pngwn
Copy link
Member Author

pngwn commented Mar 1, 2023

Hmm, let me take a look.

@pngwn
Copy link
Member Author

pngwn commented Mar 1, 2023

Okay, I think I have fixed everything now @freddyaboulton.

@freddyaboulton
Copy link
Collaborator

@pngwn Did another round of testing! I verified that all of the problems listed above are now fixed. Thank you!

Found some other things I wanted to bring to your attention:

  1. Cancelling now doesn't work in the cancel_events demo, see below that the running events are not cancelled when expected

cancel_events

  1. The Clear button for interfaces and in general the _js parameter does not seem to be working.
    You get this from the console
Blocks.svelte:364 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'queue')
    at $.<anonymous> (Blocks.svelte:364:34)
    at index.080ff49d.js:4:1266
    at Array.forEach (<anonymous>)
    at $.zo (index.080ff49d.js:4:1253)
    at $.b (Radio.svelte:17:13)
    at index.080ff49d.js:4:1054
    at Array.forEach (<anonymous>)
    at index.080ff49d.js:4:1040
    at i.$$.update (Radio.svelte:18:5)
    at Eo (index.080ff49d.js:4:1810)

I should clarify that the Clear button does clear the components but it emits that warning to the console that wasn't there before. The blocks_js_methods is not working at all for me.

js_handler_not_working
clear_console_log

  1. auth is not working? I see the Could not load the space page when running hello_login:

Screen Shot 2023-03-01 at 4 35 01 PM

Tried a bunch of demos and they all seem to be working well besides this. Will continue looking!

@pngwn
Copy link
Member Author

pngwn commented Mar 1, 2023

:[

@pngwn
Copy link
Member Author

pngwn commented Mar 1, 2023

@freddyaboulton this time it is definitely all working

@abidlabs
Copy link
Member

abidlabs commented Mar 2, 2023

Suggestion for naming:

await app.predict(payload) --> gives you the result directly

app.createJob(payload) --> sets up a job whose status you can check, add callbacks to, etc. (the current behavior of .predict)

Not sure if this nomenclature would sound famililar to JS developers, maybe we need to tweak, but this sounds about right to me since the second one creates a job while not immediately returning a prediction.

@pngwn pngwn marked this pull request as draft March 2, 2023 18:28
@pngwn
Copy link
Member Author

pngwn commented Mar 2, 2023

Will revisit later.

@pngwn pngwn closed this Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants