-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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 open in colab buttons to demos in docs and /demos #2608
Conversation
All the demos for this PR have been deployed at https://huggingface.co/spaces/gradio-pr-deploys/pr-2608-all-demos |
Wow this is great @aliabd! LGTM with a few suggestions on the points you raised:
I took a look at the
I think that's fine. I think it's pretty clear to users when this happens and how to fix it. As a general note, perhaps we should switch to using URLs in our demos instead as much as possible |
This is great. I think there should be a github action that opens a commit if a demo changes and the notebook hasn't. |
…nto aliabd/colab-buttons
…nto aliabd/colab-buttons
Ok I updated this:
|
import random | ||
|
||
GRADIO_DEMO_DIR = os.getcwd() | ||
DEMOS_TO_SKIP = {"all_demos", "reset_components", "custom_path", "kitchen_sink_random"} |
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 are we skipping "reset_components", "custom_path", "kitchen_sink_random"
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.
reset_components
and kitchen_sink_random
import from other demos so can't be used on colab alone. We also skip them when uploading to spaces because they don't work there either. They're only used for the PR demos. (along with all_demos
)
custom_path
is a demo of launching gradio using uvicorn from the terminal, so not really useful on colab either (also skipped on spaces)
Awesome PR @aliabd! Left a couple of nits above but otherwise LGTM. One suggestion -- if the CI check fails, would it be possible to print the command that needs to be run to regenerate the notebooks? |
Co-authored-by: Abubakar Abid <abubakar@huggingface.co>
The demo notebooks don't match the run.py files. Please run this command from the root of the repo and then commit the changes: pip install nbformat && cd demo && python generate_notebooks.py |
@abidlabs thanks for the suggestion. The github action will now comment on the PR if the check fails with the command that needs to be run. The last two commits show an example of this. |
The demo notebooks don't match the run.py files. Please run this command from the root of the repo and then commit the changes: pip install nbformat && cd demo && python generate_notebooks.py |
- name: Assert Notebooks Match | ||
id: assertNotebooksMatch | ||
run: git status | grep "nothing to commit, working tree clean" | ||
- name: Comment On PR |
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.
Does this run on forked repos?
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 question, I just checked on this PR #2691
The check fails as expected, but it can't comment. Do you know how to fix that? Apparently it's an open issue.
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 need to follow this approach I think: https://securitylab.github.com/research/github-actions-preventing-pwn-requests/#:~:text=The%20main%20differences%20between%20the,but%20not%20from%20external%20forks. I think it may also be possible to comment on the pr if the action is defined in a separate repo? I think that's what doc-builder does.
@pngwn May now some best practices for commenting on PRs from forks.
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.
Ah nice
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.
Super cool @aliabd !
content = json.loads(content) | ||
for i, cell in enumerate(content["cells"]): | ||
random.seed(i) | ||
cell["id"] = random.getrandbits(128) |
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.
Just wondering why this is neccessary?
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 question...
The way the github check works is by running python generate_notebooks.py
and seeing if there's anything to commit. If there is, it must mean that run.py files and run.ipynb notebooks don't match.
The problem is that jupyter notebooks create a random id for every cell that's different each time, and creates a huge mess on git diffs even when the notebook is functionally the same. So everytime python generate_notebooks.py
is run it will show that every notebook has changed. There's no way to set the id using nbformat so instead I just manually set them in the file from python and define a seed to make sure they're always the same.
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.
Very cool - thanks for explaining!
@freddyaboulton I updated this to use |
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.
looks great!
Adds buttons to open a demo in colab:
Untitled.mov
If a contributor wants to change a demo in the /demo dir, they just have to run /demo/generate_notebooks.py to regenerate all the .ipynb notebooks. I could create a github action for this but seems like overkill.
Note that not all the demos will work on colab, the ones that reference another file in the dir won't work (for example). I could detect this and not show a colab button for those, but I feel like it would still be useful for a reader to be able to play with the code (so don't know if it's the right idea to skip them), and usually you can easily replace the file (I could link to the dir in github so they can move the files they need?)
Closes: #2278
Checklist: