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
[WIP] Refactor file normalization to be in the backend and remove it from the frontend of each component #7183
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.
|
…o refactor-normalise
client/js/src/upload.ts
Outdated
@@ -18,46 +18,18 @@ export function normalise_file( | |||
proxy_url: string | null // root_url: string | null | |||
): FileData[] | FileData | null; | |||
|
|||
/* |
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.
Is there a need for backwards compatibility? wouldn't previous custom components be version pinned for the client?
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.
They should but we would need to make the client version bump minor, to respect semver.
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.
(Minor because this is a pre-1.0 version, so minor is breaking).
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.
Good point @aliabid94 @pngwn I'll just remove this method altogether then and change to minor
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.
Done
@@ -64,8 +62,12 @@ | |||
share: ShareData; | |||
}>; | |||
|
|||
$: url = _value?.url; | |||
$: url, gradio.dispatch("change"); | |||
$: { |
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.
why not just listen to the url change as before?
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.
Yeah that would be okay too, I ended up removing quite a few things from Image, including this line, and then I copied this snippet over from the Audio component. Either way should be fine
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.
LGTM, nice cleanup
Thanks for the review @aliabid94! I'm going to merge this in as it'll make it easier to tackle some other issues I'm looking at. |
The idea behind this PR is to clean up how we do "file normalization" i.e. how we convert filepaths into URLs like
/file=filepath
or/proxy=filepath
so that the files can be fetched from the frontend of the Gradio app.A filepath is converted into a URL in the following way:
If its a file on the local machine, it is prepended with
{root_url}/file=
. The root url is the path where the Gradio app is running. Usually, it is justhttp://localhost:7860
. However, if the Gradio app is running within a larger FastAPI app on a subpath, or if it is running behind an nginx server, the root_url might be different, e.g.http://localhost:7860/gradio
.Otherwise, if the filepath is on an external Space (in the words, the component has been loaded using
gr.load
), then the prefix{root_url}/proxy={space_url}/file=
is prepended to the filepath, which makes it a URL that can be accessed from the frontend. The local Gradio app serves as a proxy to route the request, as this is needed to access files on private Spaces using the local user's HF tokenPreviously, this file normalization was done in the frontend, in every component's frontend files, which led to code repetition and error-prone code, and was an annoyance for custom component developers. This PR moves it to the backend and does it in a single place, in a backwards-compatible manner for existing custom components.
Once we have this cleanup in place, it should be easier to tackle issues like: #6744
Todo: