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

URL validation/access fixes #4695

Merged
merged 6 commits into from
Jun 30, 2023
Merged

URL validation/access fixes #4695

merged 6 commits into from
Jun 30, 2023

Conversation

akx
Copy link
Contributor

@akx akx commented Jun 27, 2023

Description

I was profiling things and noticed a whole lot of time was spent in is_valid_url and was pretty surprised that that function actually does HTTP requests if it can.

This PR:

  • makes that function share a requests session if it needs to do both HEAD + GET
  • renames that function to probe_url because that's more what it does, with a deprecation trampoline for possible external users
  • changes uses where is_valid_url was only used to check whether another function that does a requests request should be called to the new is_http_url_like function.

While working on that I realized there were small bugs in encode_url_to_base64 and download_tmp_copy_of_file where they didn't check for the response status, and would thus happily end up writing e.g. an error page out.

These changes are in the gradio_client library, but these functions are gratuitously used by gradio itself.

🎯 PRs Should Target Issues

This PR does not target an issue right now, sorry; this was just another small papercut sort of problem I could easily fix.

@gradio-pr-bot
Copy link
Collaborator

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

@akx akx force-pushed the is-valid-url branch 2 times, most recently from e5f04d7 to 500dc72 Compare June 27, 2023 18:10
@akx akx changed the title Rename is_valid_url to probe_url URL validation/access fixes Jun 27, 2023
@akx akx marked this pull request as ready for review June 27, 2023 18:23
@abidlabs
Copy link
Member

Nice improvements @akx!

Could we just add some tests for this:

While working on that I realized there were small bugs in encode_url_to_base64 and download_tmp_copy_of_file where they didn't check for the response status, and would thus happily end up writing e.g. an error page out.

Should be good to merge then

@abidlabs
Copy link
Member

Thank you very much @akx!

@abidlabs abidlabs merged commit 2544e7b into gradio-app:main Jun 30, 2023
@pngwn pngwn mentioned this pull request Jul 26, 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
Development

Successfully merging this pull request may close these issues.

3 participants