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

Add Gradio support #2418

Merged
merged 1 commit into from
Dec 10, 2024
Merged

Add Gradio support #2418

merged 1 commit into from
Dec 10, 2024

Conversation

edavidaja
Copy link
Collaborator

Intent

Add support for deploying gradio applications.

Type of Change

    • Bug Fix
    • New Feature
    • Breaking Change
    • Documentation
    • Refactor
    • Tooling

Approach

Automated Tests

Added.

Directions for Reviewers

Obtain a sample gradio application and deploy it to Connect.

Checklist

@dotNomad
Copy link
Collaborator

dotNomad commented Nov 6, 2024

Note that we will want to wait to merge this until the Connect release with Gradio support is released.

@dotNomad dotNomad changed the title gradio support Add Gradio support Nov 6, 2024
@dotNomad dotNomad added the hold label Nov 6, 2024
@dotNomad dotNomad added this to the v1.6.0 milestone Nov 6, 2024
Copy link
Collaborator

@dotNomad dotNomad left a comment

Choose a reason for hiding this comment

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

I was able to successfully deploy the hello world example however a few things:

On the Connect side I got errors, and it didn't behave as expected. I have a theory this is because of Connect, and not necessarily Publisher, but I'll want to verify that.

Additionally I think there is a question about if we want to add this to our documentation in the state this is currently in without inspection when selecting a gradio Python file

That isn't a criticism of your work @edavidaja but a question on how to handle this PR and when to merge it / announce it.

@jonkeane
Copy link
Collaborator

jonkeane commented Nov 7, 2024

without inspection when selecting a gradio Python file

Presumably this is either something we add here or follow on with a PR? Or am I missing something else?

@dotNomad
Copy link
Collaborator

dotNomad commented Nov 7, 2024

without inspection when selecting a gradio Python file

Presumably this is either something we add here or follow on with a PR? Or am I missing something else?

No you aren't missing anything. I just think we need to make a decision on:

  • do we think that updating the docs and communicating the gradio type to exists without our inspection helping is acceptable
  • if not do we pick up the work to add that inspection before this merges / open another PR

@marcosnav
Copy link
Collaborator

do we think that updating the docs and communicating the gradio type to exists without our inspection helping is acceptable

IMO, It does not feel complete without the inspection part.

Another thing to consider, how does the publisher behave when publishing a gradio type app to a Connect instance that does not support it?. I'm assuming this is the first time the publisher adds a new content type that many Connect instances might not support?

@dotNomad
Copy link
Collaborator

dotNomad commented Nov 7, 2024

Another thing to consider, how does the publisher behave when publishing a gradio type app to a Connect instance that does not support it?

This is great question. Ideally what it should do is propagate the error we get back from Connect which should be meaningful about not supporting that content type.

We are getting into a bit of a larger design discussion here, but from what it sounds like we should be able to merge this, but perhaps hold off on including it in the changelog until the Gradio support is complete. We could create an epic on the board to track that.

@jonkeane
Copy link
Collaborator

jonkeane commented Nov 8, 2024

We are getting into a bit of a larger design discussion here, but from what it sounds like we should be able to merge this, but perhaps hold off on including it in the changelog until the Gradio support is complete. We could create an epic on the board to track that.

This sounds good. Yes, in this case we definitely do not want to advertise this too heavily until it's officially added to Connect and that is released.

@dotNomad dotNomad removed this from the v1.6.0 milestone Nov 13, 2024
@sagerb
Copy link
Collaborator

sagerb commented Dec 6, 2024

Another thing to consider, how does the publisher behave when publishing a gradio type app to a Connect instance that does not support it?

Our approach has been so far, (following the goal of failing as fast as possible), to do the check within our prechecks, rather than wait for a server failure (which often comes back pretty raw - this one would come back as an invalid content type?).

We are already detecting if functionality is available or not within the functionality of internal/clients/connect/capabilities.go, and it would seem proper to add a check within the CheckCapabilities call.

For example, we are already checking within that code if the target server supports a Python deployment. I see a strong parallel with Gradio here.

Does a Connect server that supports Gradio offer any way of querying its support via an API?

@dotNomad dotNomad added this to the v1.6.0 milestone Dec 8, 2024
@dotNomad dotNomad self-assigned this Dec 8, 2024
@dotNomad dotNomad removed the hold label Dec 8, 2024
@dotNomad
Copy link
Collaborator

dotNomad commented Dec 8, 2024

The code looks good, but I am still seeing errors in Connect when I deploy the Hello World gradio example. @edavidaja lets chat on Monday to figure out what is going on there, and we will get this PR buttoned up.

@sagerb
Copy link
Collaborator

sagerb commented Dec 10, 2024

FYI, I'm working on an issue that will require a schema change (moving to v4 - new file). Since this PR includes an addition to the allowed content types, we'll need to coordinate the changes or we may lose a change. issue #2468.

Copy link
Collaborator

@dotNomad dotNomad left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to diagnose this with me @edavidaja

For those following along it was an older version of Gradio specified in the requirements.txt. Un-bounding that and getting a newer version fixed the errors I was seeing on the Connect side.

Approving and merging 🎉

@dotNomad dotNomad merged commit 956e4be into main Dec 10, 2024
14 checks passed
@dotNomad dotNomad deleted the eda-gradio-support branch December 10, 2024 20:21
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.

5 participants