gradio-app / gradio

Build and share delightful machine learning apps, all in Python. 🌟 Star to support our work!
http://www.gradio.app
Apache License 2.0
30.36k stars 2.26k forks source link

Fix zero gpu auth ignoring hf_token #8323

Closed Saghen closed 4 weeks ago

Saghen commented 1 month ago

Description

When sending requests to a ZeroGPU endpoint via the JS client on node, the client would attempt to index window while getting credentials, causing an error. The logic has been changed to prefer the hf_token, falling back to the post_message if the window is available, and finally falling back to null.

I have not included an issue due to the size of the PR, as per the PR template, but will add if needed!

gradio-pr-bot commented 1 month ago

🪼 branch checks and previews

• Name Status URL
Spaces ready! Spaces preview
Website ready! Website preview
:unicorn: Changes detecting...

Install Gradio from this PR

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

Install Gradio Python Client from this PR

pip install "gradio-client @ git+https://github.com/gradio-app/gradio@ee05c5e1bb523eae5c8483c0afe00c872d148688#subdirectory=client/python"
cbensimon commented 1 month ago

Nice catch @Saghen!

One point though: isn't the Authorization header already set elsewhere in the client ? zero_gpu_auth only aims to add additional headers specifically supported by the ZeroGPU Spaces infra.

Otherwise, totally agreed that we should make the code Node compatible so checking the window object seems reasonnable. @abidlabs do you confirm that the JS client should target NodeJS ? Shouldn't the CI somehow check the compatibility with Node then ?

gradio-pr-bot commented 1 month ago

🦄 change detected

This Pull Request includes changes to the following packages.

Package Version
@gradio/client minor
gradio minor

With the following changelog entry.

Fix zero gpu auth ignoring hf_token

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](https://github.com/Saghen/gradio/edit/fix/zero-gpu-auth/.changeset/plenty-socks-relate.md).
abidlabs commented 1 month ago

@abidlabs do you confirm that the JS client should target NodeJS ? Shouldn't the CI somehow check the compatibility with Node then ?

Let me tag @hannahblair who can review this PR better than me

Saghen commented 4 weeks ago

One point though: isn't the Authorization header already set elsewhere in the client ? zero_gpu_auth only aims to add additional headers specifically supported by the ZeroGPU Spaces infra.

Yep, thanks! Removed that logic

pngwn commented 4 weeks ago

@cbensimon

@abidlabs do you confirm that the JS client should target NodeJS ?

The client should (and will eventually) work on all platforms, not just node + browser but also cf workers, service workers, deno, etc.

Shouldn't the CI somehow check the compatibility with Node then ?

Yes, and it kind of does but we don't have any tests for zero gpu atm.

cbensimon commented 3 weeks ago

Yes, and it kind of does but we don't have any tests for zero gpu atm.

@pngwn I was talking about static (typing) tests (they should catch this "window is possibly undefined" thing I think)

pngwn commented 3 weeks ago

Since it has to work in both environments it isn't an error to reference the window as sometimes that is present. We don't run type checks twice (once for each environment).

cbensimon commented 2 weeks ago

it isn't an error to reference the window as sometimes that is present

Since we want both browser and NodeJS compatibility, to me it is actually an error to not check for existence before using the window object (and I think that typescript can be configured to handle this and make projects both browser and NodeJS compatible but I'm not 100% sure)