-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Allow gr.Request
to work with ZeroGPU
#9148
Conversation
🪼 branch checks and previews
|
🦄 change detectedThis Pull Request includes changes to the following packages.
With the following changelog entry.
Maintainers or the PR author can modify the PR title to modify this entry.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach looks good but I'm worried that not all the attributes from the base request will be copied over. Maybe we can also copy url
and state
?
@@ -177,6 +181,23 @@ def __getattr__(self, name): | |||
) from ke | |||
return self.dict_to_obj(obj) | |||
|
|||
def __getstate__(self) -> dict[str, Any]: | |||
self.kwargs.update( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably set request.request
when its unpickled, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. There's no additional value in setting it since if its not set, the attributes that a user fetches from gr.Request
are served from self.kwargs
instead of self.request
, which is what we want. That's the logic here:
def __getattr__(self, name: str):
if self.request:
return self.dict_to_obj(getattr(self.request, name))
else:
try:
obj = self.kwargs[name]
except KeyError as ke:
raise AttributeError(
f"'Request' object has no attribute '{name}'"
) from ke
return self.dict_to_obj(obj)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I meant it more so that the api of a Request object in ZeroGPU is the same as outside ZeroGPU. Like someone used to doing request.request.client.host
would get an error in ZeroGPU. Not sure if that's common though so we can keep as is.
Good idea, I'll add url, state (if pickleable), path_params, and cookies (Btw I tried a more generic approach of just serializing all the attributes of |
Ok added a few more attributes, which I think should cover the vast majority of cases. Will merge after CI runs, thanks @freddyaboulton. |
Sorry, looks like I came here a bit too late. import gradio as gr
import spaces
@spaces.GPU
def fn(request: gr.Request):
print(request)
with gr.Blocks() as demo:
btn = gr.Button()
btn.click(fn=fn)
demo.queue().launch() |
Wonderful thanks for confirming @hysts! Btw you don't need |
Ah, good to know. I remember seeing that discussion (before version 4?), but I wasn't sure how it was actually implemented because I also remember seeing discussions about enabling it by default on Spaces or on ZeroGPU Spaces in a bit different contexts, and I've just been using the old code. Thanks for the info! |
Closes: #8844. So the underlying issue is that the
fastapi.Request
class is not pickleable. Making it pickleable is tricky because it contains lots of attributes that are custom classes, which themselves are not serializeable. So what I've done here is extract the main attributes that we mention in our docs are users are likely using and ensured that they are preserved when thegr.Request
object is picked and unpickled. I've added unit tests to confirm this. @hysts, quick favor -- if you have a chance to test on ZeroGPU directly, that'd be awesome.