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

Fix OAuth + fix OAuth documentation + undocument logout button #7697

Merged
merged 3 commits into from Mar 14, 2024

Conversation

Wauplin
Copy link
Contributor

@Wauplin Wauplin commented Mar 14, 2024

2 things in this PR:

  1. Fix using gr.LogoutButton breaks app #7695. The snippet example in the OAuth docs is not complete, resulting in a runtime error when running it. I forgot from huggingface_hub import whoami and demo.launch(). This is now fixed and I've been able to run it both locally and in a Space. I also took the opportunity to remove mentions to gr.LogoutButton from the docs (deprecated in Single oauth button #7063).
  2. There seem to be a major bug that broke all OAuth apps on the latest version of gradio. For some reason, now request.session is not a dictionary anymore but a Obj object that mimics it. The Obj class has been added 6 months ago but the bug is for sure much more recent. Can it be related to Allow mounted Gradio apps to work with external / arbitrary authentication providers #7557 cc @abidlabs? In any case, I've added a get method to Obj which solved the issue.

I think that once this PR is merged, we should make a follow-up release given that it affects all oauth apps running on the latest version of Gradio 😕

cc @pngwn who pinged me on this.

@gradio-pr-bot
Copy link
Contributor

gradio-pr-bot commented Mar 14, 2024

🪼 branch checks and previews

Name Status URL
Spaces ready! Spaces preview
Website ready! Website preview
🦄 Changes detecting...

Install Gradio from this PR

pip install https://gradio-builds.s3.amazonaws.com/6ef5b2640fbdc37510fa11eed1b0f00875fdaf32/gradio-4.21.0-py3-none-any.whl

Install Gradio Python Client from this PR

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

@gradio-pr-bot
Copy link
Contributor

🦄 change detected

This Pull Request includes changes to the following packages.

Package Version
gradio patch
  • Maintainers can select this checkbox to manually select packages to update.

With the following changelog entry.

Fix OAuth + fix OAuth documentation + undocument logout button

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.

@Wauplin Wauplin mentioned this pull request Mar 14, 2024
1 task
@abidlabs
Copy link
Member

Thanks @Wauplin for this PR! Taking a look now

There seem to be a major bug that broke all OAuth apps on the latest version of gradio. For some reason, now request.session is not a dictionary anymore but a Obj object that mimics it. The Obj class has been added 6 months ago but the bug is for sure much more recent. Can it be related to #7557 cc @abidlabs? In any case, I've added a get method to Obj which solved the issue.

That's really strange. I don't think #7557 should have affected request.session in any way. As you said, the Obj class was introduced a long time ago and it should generally just operate like a dictionary. I'll see if I can trace the regression.

@abidlabs
Copy link
Member

@Wauplin are you sure that OAuth was broken? I just tested here with the version on main and it seems to be working fine: https://huggingface.co/spaces/abidlabs/oauth_test3

@@ -82,6 +82,11 @@ def __contains__(self, item) -> bool:
return True
return False

def get(self, item, default=None):
Copy link
Member

Choose a reason for hiding this comment

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

Trying to understand if we need this? Where do we call .get() on the request object? Note that we already have a __getitem__ which should provide a basic way to let this class be used as a dictionary

Copy link
Contributor Author

@Wauplin Wauplin Mar 14, 2024

Choose a reason for hiding this comment

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

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.

@Wauplin this PR looks good and I've confirmed that OAuth continues to work: https://huggingface.co/spaces/abidlabs/oauth_test4

That being said, I can't repro OAuth being broken in the first place. Would be good to clarify that before merging in.

@Wauplin
Copy link
Contributor Author

Wauplin commented Mar 14, 2024

@abidlabs here is an example running on Gradio 4.21.0: https://huggingface.co/spaces/Wauplin/tmp_test_oauth. I'm getting an error when Gradio internals try to get oauth information from request.session that is no longer a dict but an Obj:

image

Here is the traceback that helped me debug it:

Traceback (most recent call last):
  File "/usr/local/lib/python3.10/site-packages/gradio/queueing.py", line 501, in call_prediction
    output = await route_utils.call_process_api(
  File "/usr/local/lib/python3.10/site-packages/gradio/route_utils.py", line 253, in call_process_api
    output = await app.get_blocks().process_api(
  File "/usr/local/lib/python3.10/site-packages/gradio/blocks.py", line 1695, in process_api
    result = await self.call_function(
  File "/usr/local/lib/python3.10/site-packages/gradio/blocks.py", line 1221, in call_function
    processed_input, progress_index, _ = special_args(
  File "/usr/local/lib/python3.10/site-packages/gradio/helpers.py", line 792, in special_args
    oauth_info = session.get("oauth_info", None)
AttributeError: 'Obj' object has no attribute 'get'

By a quick git blame, it looks like this line has been updated here (in #7495, 3 weeks ago).
Adding get method in Obj solves the problem (sorry not mentioning it in the first place in this PR 😬)

@abidlabs
Copy link
Member

Ah that explains it. We refactored this assuming that request.session is a dictionary. Nice catch @Wauplin and thanks for explaining. We'll get a release out soon.

@abidlabs abidlabs merged commit a1c24db into gradio-app:main Mar 14, 2024
7 checks passed
@Wauplin
Copy link
Contributor Author

Wauplin commented Mar 14, 2024

I was also expecting it to be a dictionary 😁

@pngwn pngwn mentioned this pull request Mar 14, 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.

using gr.LogoutButton breaks app
3 participants