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

Suggestion: Manage notebooks in common way #2

Open
kosack opened this issue Dec 12, 2018 · 9 comments
Open

Suggestion: Manage notebooks in common way #2

kosack opened this issue Dec 12, 2018 · 9 comments

Comments

@kosack
Copy link
Collaborator

kosack commented Dec 12, 2018

I love the idea of benchmarks being a set of Jupyter notebooks, so they are easier to add and read.

The problem with notebooks in GIT is always:

  • git diffs don't mean much, especially if output is in the notebook
  • small changes change metadata, etc
  • code style is hard to maintain

There is a nice solution from the GammaPy developers: their gammapy jupyter command lets you manage a set of notebooks in a common way:

  • gammapy jupyter strip : removes output, so diffs make sense (use nbdime diff for even better diffs)
  • gammapy jupyter black runs black code formatter on cells in all notebooks

etc.

Then they only commit stripped noteboooks to git, and automatically run the notebooks producing documentation in Sphinx format for viewing.

Can we re-use this technique? Or perhaps ask them to split out that functionality into a stand-alone tool, so gammapy is not needed?

@vuillaut
Copy link
Member

We surely can use this technique there.

So the output of the notebooks are visible in the documentation? That's great.
Though it means having datafiles available. For a lot of benchmarks, I think some statistics will be needed. But you were suggesting to have external files accessible for ctapipe also, so the same strategy could be applied here.

The best thing would be to have this functionality directly from jupyter. Maybe the guys who developed this technique could make a PR?

@kosack
Copy link
Collaborator Author

kosack commented Dec 12, 2018

Yeah, we probably need a dedicated server with the data where we can run all of these via a CI system and upload the results to a webserver eventually (could be internal).

@kosack
Copy link
Collaborator Author

kosack commented Dec 12, 2018

We might want to look at this example from what Netflix (!) is doing with automated notebooks:

It could serve as a nice platform for automated benchmarking for each version release.

Thanks to @adonath for mentioning this.

@adonath
Copy link

adonath commented Dec 12, 2018

We should definitely add @Bultako to the discussion here, as he did most of the work on gammapy jupyter and also mentioned papermill to me the first time.

@Bultako
Copy link

Bultako commented Dec 12, 2018

Hi!
Thanks for adding me to this thread. Just some comments in case it helps.

Can we re-use this technique? Or perhaps ask them to split out that functionality into a stand-alone tool, so gammapy is not needed?

It is not a big effort to split out gammapy jupyter CLI into a stand-alone tool, and as suggested, I would also prefer to have this directly integrated into the standard jupyter command.

The best thing would be to have this functionality directly from jupyter. Maybe the guys who developed this technique could make a PR?

We have filed several issues and comments in the jupyter nbconvert repo.

So the output of the notebooks are visible in the documentation?

Yes.
We integrate the output-filled executed notebooks into the versioned gammapy docs with nbsphinx.

Yeah, we probably need a dedicated server with the data where we can run all of these via a CI system and upload the results to a webserver eventually (could be internal).

In Gammapy we do the CI with Travis and fetch the needed datasets from a Github repo during the CI building. The notebooks are tested with gammapy jupyter test. Roughly, we execute the notebook with jupyter nbconvert and then we parse the output cells, in case we found and error in one of the output cells we say that the test has failed.

https://github.com/gammapy/gammapy/blob/48a822faa648b85b2af2e8a822bb6c62d9eeb9eb/gammapy/scripts/jupyter.py#L161

It is a very simple method, we do not assert values like it's done in nbval, and we can test with different kernels (gammapy versions)

--.

Papermill seems great.
I have not used it, but I see the potential of

  • executing parametrized notebooks
  • composing workflows based on the orchestration and output of several notebooks
  • modularity - reusing code and outputs from other notebooks

(other) cons using notebooks:

  • It is relatively easy to end up with complex, non-linear and longish notebooks, sometimes with high executions times, then not really useful as richly documented executable recipes for users.
  • When high executions times are needed, I don't know how reliable the notebook execution server could be.

I copy here the useful guidelines exposed in the Netflix post

  • Low Branching Factor: Keep your notebooks fairly linear. If you have many conditionals or potential execution paths, it becomes hard to ensure end-to-end tests are covering the desired use cases well.
  • Library Functions in Libraries: If you do end up with complex functions which you might reuse or refactor independently, these are good candidates for a coding library rather than in a notebook. Providing your notebooks in git repositories means you can position shared unit-tested code in that same repository as your notebooks, rather than trying to unit test complex notebooks.
  • Short and Simple is Better: A notebook which generates lots of useful outputs and visuals with a few simple cells is better than a ten page manual. This makes your notebooks more shareable, understandable, and maintainable.

@kosack
Copy link
Collaborator Author

kosack commented Jan 28, 2019

I've re-organized the notebooks (there were only 2 so far) into a more clean structure, and make a better README based on this discussion. I'll open some separate issues for the parts that need improvement (e.g. the build script, an example of how to do a summary, where to put library function code, etc).

@kosack
Copy link
Collaborator Author

kosack commented Jan 28, 2019

Also, the tests now use papermill instead of gammapy, but we still don't have a nice way to strip the output unless we also install gammapy (or just require that people committing should strip them). I've enabled the requirement that we have a code review before accepting a PR, so we could just enforce it that way. The two notebooks that are included so far break both rules (they use data committed to the repo, which is no longer allowed, and the output is still not stripped).

@kosack
Copy link
Collaborator Author

kosack commented Jan 28, 2019

it seems the nbstripout tool can be used instead of gammapy's solution: conda install -c conda-forge nbstripout

@Bultako
Copy link

Bultako commented Jan 28, 2019

FYI --

We do have some issues with the uncertainties package in Gammapy, potentially related with installing nbstripout (and/or other external packages not related with gammapy), which changes the version of numpy (among possibly other things).

gammapy/gammapy#2007 (comment)

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

No branches or pull requests

4 participants