-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Update JAX Quickstart with parallelization (pmap
), other changes
#3520
Conversation
Blah, this breaks RTD because it can't run the notebook without a Cloud TPU :( I think this means we have to choose between pmap examples (requires multiple devices --> Cloud TPU) and automated testing/generation of the Quickstart notebook via RTD. I personally think the pmap examples are worth it (how often do we break the quickstart notebook?), but I could be convinced otherwise. @gnecula @shoyer do have any alternate ideas, or opinions on what we should do here? (I was wondering if we could add conditional guards so RTD only runs what it can, but I think this means we wouldn't have pmap output at https://jax.readthedocs.io/en/latest/notebooks/quickstart.html.) |
@skye just a thought: Would adding the quickstart to exclude_patterns = [
# Slow notebook: long time to load tf.ds
'notebooks/neural_network_with_tfds_data.ipynb',
# Slow notebook
'notebooks/Neural_Network_and_Data_Loading.ipynb',
'notebooks/score_matching.ipynb',
'notebooks/maml.ipynb',
# Fails with shape error in XL
'notebooks/XLA_in_Python.ipynb',
# Sometimes sphinx reads its own outputs as inputs!
'build/html',
] Also, to keep the original link/reference on the ReadTheDocs site, you could manually edit https://github.com/google/jax/blob/master/docs/index.rst or something like that? I'm not too familiar with this process yet. Are there any plans to add any |
My understanding is that adding the quickstart to I don't think there are any immediate plans for a |
@skye I can set the default Colab notebook accelerator to GPU and move the extra TPU settings cell down to the |
RTD can only execute using a CPU. So we can't use |
@shoyer maybe I can switch the default hardware in the Colab notebook to CPU while keeping the output. Then commit that new ipynb here. We can add a note that says if you want to run pmap, click Open in Colab at the top of the page. WDYT |
With Given the lack of
Read the Docs build should recognize this as simple markdown. It's a temp workaround. (Further down the line: I can investigate how to run Notebooks in other OS doc building solutions, such as Hugo with Google's Docsy theme - used by Kubeflow and Kubernetes projects. Apparently, their build speed is good (the site is static too), and Hugo may have notebook-integration.) |
@8bitmp3 thanks for your work on this! I'll let @skye take the lead on reviewing. But I did want to note that you might want to take a look at the rendered version of the notebook by following the "docs/readthedocs.org:jax" link under "Checks" : https://jax--3520.org.readthedocs.build/en/3520/notebooks/quickstart.html One thing you'll notice is that unfortunately formatted links don't work. This appears to be a limitation of reStructuredText: spatialaudio/nbsphinx#301 |
@skye Check out the new commit that fixes external link rendering in RTD/Sphinx (see details below). Let me know what you think about this and other workarounds. I've built the Docs site locally and all looks OK so far. Thanks @shoyer for spotting this link issue in Sphinx/reStructuredText. Another JAX notebook here had a similar issue that I spotted recently and fixed by attaching the hyperlinks to plain words (see 86fcc77 and f351da7) In the Quickstart, I use the following format for functions and methods that have API doc links:
which will look like plain text with round brackets: e.g. ohFunctionMyFunction() — notice the lack of back ticks. They causes the original issues (see below). On the other hand, they're blue and not red. More background: For external links, Sphinx follows this rule, which already uses back ticks:
Meanwhile, this clashes with standard Markdown, which Sphinx supports, just not fully: This is normal Markdown outside of RTD/Sphinx:
[`pmap`](https://jax.readthedocs.io/...) (See: https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#external-links) So, adding more back ticks around the name of functions that we want to link the API docs to only breaks the sequence/links/everything else. For everything else that doesn't require external links, we leave them as before: |
I like the explicit instructions on how to enable the TPU code! It has the added benefit of teaching new users how to use Cloud TPU colabs :) However, I still think it'd be better to manually generate the notebook so we can have actual code cells + output in the pmap section, instead of requiring users to copy/paste. There's nothing here that's expected to change, so I think the added usability is worth the slight risk of a stale notebook. So specifically, add quickstart.ipynb to Also, even though we could switch to TPU at the beginning with this scheme, I like how you do it here and switch just above the pmap examples. That way readers won't be scared away by the preamble at the beginning, and we get the added educational benefit. WDYT? |
@skye That's awesome, thank you so much for the feedback 🙏
New issue: If you set the default hardware to TPU in the beginning and run @jit, @Grad, and @pmap stuff—everything is fine as expected. After running these computation, setting the TPU driver as the backend creates a new issue: you get only one device. If you run a
Solution 1: Run the notebooks with CPUs until you reach Solution 2: If TPU is enabled from the beginning, run the TPU driver backend config cell first, perform the rest of the calculations thereafter 🤔 Solution 2 creates a less desirable UX by undoing this:
WDYT Thank you! (What if TPUv2 allocation is carried out with RL (Hierarchical Planning for Device Placement.)) |
Let's leave out the NN library references. Unfortunately it's pretty hard to review notebook changes, so I'd like to keep this change as minimal as possible (I haven't actually looked at all the individual edits you've made yet, I'll do that once we get the overall structure nailed down). Plus the rest of the core team / NN library authors might have opinions about that :) For the runtime issue, I agree Solution 1 is better (let's run on GPU until we switch to TPU though, so the microbenchmarks still look good). Also note you may have to
😆 |
Done @skye. New:
As before:
Thanks @skye. It's ready for your review. |
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.
Update READMEs to "2020"
We don't update copyrights actually, I guess they're supposed to reflect when the file was first created.
Add a reference to the notebook in docs/index.rst
Can you also put a link at the top of the Tutorials section immediately below? Basically how it is currently. I think it's good to have it listed there too for people like me who skim over prose :) And also to have a link to the HTML version.
The overall structure looks good, but I'm having trouble reviewing the exact changes to the notebook because there are so many, and it's hard to review notebooks. Would it be possible to separate this out into two changes, one containing the changes to the notebook above the new pmap section, and one with everything else (the new pmap section, disabling RTD, etc.)? Probably the pmap + Cloud TPU change should come first. If it's a big pain to split it up at this point I can review the whole thing (maybe with the help of ReviewNB, lemme know.
Hmm I guess posting the above comment also published the in-file comments from when I started reviewing, before realizing this would take a long time. I deleted them to avoid confusion. |
To make the review process easier @skye I’ll reverse the commits and then introduce separate ones for |
@skye @shoyer I may have found a long-term solution to working with
The current JAX site generator is Sphinx that uses on reStructured text. Like Sphinx, MkDocs is also recommended by ReadTheDocs. Unlike Sphinx though, it uses Markdown. For Jupyter notebooks, there's a plugin that allows to not only render Below is a screenshot from a simple docs site I built. This is a the The docs site is stored in a By the way, the search is fast—it shows the results as you type: |
Fyi, It's possible to use MkDocs instead of Sphinx and keep the ReadTheDocs theme (it comes pre-loaded and you set it as Below is another example with the blue color scheme in the
This is the JAX+TFDS notebook that's not pointing to an external link like it currently does (➡️ https://github.com/google/jax/blob/master/docs/notebooks/neural_network_with_tfds_data.ipynb). You're simply viewing the notebook in |
Another suggestion @skye @shoyer
I got this idea because in MkDocs (a Sphinx alternative) there's a plugin called MkDocs Jupyter that allows to:
And in Sphinx - where you use the So, in
For example, this is the Auto-Batching Log-Densities Example notebook that shows which cells were run by the original author before saving the notebook: (The notebook cells weren't run by Sphinx/ If this works for all the notebooks, then, technically, you won't need to add a bunch of notebooks to
WDYT? |
To enable TPU support in notebooks with the `pmap` transform in Sphinx, notebooks should not be executed during build before conversion. Here, you change 'always' to 'never', such that `nbsphinx_execute = 'never'`.
@skye I've refactored the JAX Quickstart notebook in stages with minimum changes to the original layout of the site to help you with the review: Commits 1 d262a3d and 2 74e1d61 address this:
For Jupyter notebooks with TPU mentioned in the JSONs - commits 3 4115c01 and 4 728dd96 address this:
I have built and run the site locally, and the Sphinx (RTD)'s And it looks like so far there's not need in any workarounds, such as:
I think if this works for you, the other notebooks that are currently in LMKWYT Thanks 🙌 |
Thanks for separating this out, and for looking into solutions for the TPU execution!
We intentionally execute notebooks as part the docs build process to make sure they're up-to-date and to block submitting PRs that break the notebooks. Ideally we'd be able to execute all notebooks, but as you've noticed, some of the notebooks are too slow to generate this way. So we definitely don't want to set
I like these material themes, but let's try to limit the scope of this change :) I'll take a look at the specific changes separately. |
Hi @skye 👋 Hope y'all are safe and stuff!
Let's finish this task. How flexible is the team when it comes to configuring Sphinx in general? Or have y'all resolved the Cloud TPU incompatibility? |
Hi, sorry for the delay on this. I got swamped with some other work (and was on vacation for the last week), but agree we should get this done! To make this simpler, can you remove the "2-Expand the intro, ..." commit from this PR and and post it as a separate PR? That will make each PR much more self-contained and quicker to review, especially if others have opinions. And also change "Update JAX Quickstart with parallelization ( I'm actually out-of-office most of today too, but will try to review the pmap notebook changes this week. |
We can revisit in a set of small PRs @skye 👍 |
PR ref: #3458
pmap
(using accelerators in parallel) with the material from @skye NeurIPS 2019 demo, main JAX GitHub README,pmap
API docsvmap
) - please check if it's fineas jnp
instead ofnp
in line with other examples (we have ordinary NumPy asonp
, so it may be confusing otherwise for new users)vmap
is a lot like a regular Pythonmap
(but...)device_put
,vjp
,@jit
etc etcMost of these are just UX improvement suggestions. Feel free to rip them apart/propose new changes/propose to keep the old stuff.
cc @mattjj @shoyer
Thanks for your work 👍