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

Intro tutorial part2 #293

Merged
merged 28 commits into from
Feb 24, 2022
Merged

Intro tutorial part2 #293

merged 28 commits into from
Feb 24, 2022

Conversation

rwegener2
Copy link
Member

@rwegener2 rwegener2 commented Feb 18, 2022

What has been built?

This PR address the second part of Issue #280. It builds on PR #281 by creating Part 2 of an updated Introduction Tutorial sequence. Part 2 focuses on Running a recipe locally.

How was it done?

  • Created a new folder introduction_tutorial with the 1st part of the new tutorial
  • updatedindex.md for the intro tutorials (1 more part to come)

How can it be tested?

Clicking "Details" for the docs test that gets run by CI.

Notes

chunksize and prunning

In Part 1 of the recipe we told the user to use XarrayZarrRecipe(pattern, inputs_per_chunk=10). Once the recipe gets pruned and run, though, it errors with AssertionError: invalid chunksize 10 and dimsize 2. I can change inputs_per_chunk to 2 to stop this error, but I wanted to flag it in case others had better ideas. If the only use of pruned recipes is to run test chunks, it might be advantageous one day to allow pruned to skip over that error. On a shorter term basis, though, if anyone has different ideas from switching to inputs_per_chunk=2 I'm all about it.

consolidated metadata warning

When running the recipe function (recipe_pruned.to_function()()) a RuntimeWarning: Failed to open Zarr store with consolidated metadata, falling back to try reading non-consolidated metadata. warning appears. It's not the end of the world but if anyone knows what I could change to stop the warning that would be great.

logging output

The logging output is quite long and my current opinion is that it disrupts the flow of the tutorial without much gain. To deal with this I commented out the # setup_logging() line. This looks awkward in it's own way, though, so I'm not sure which way to go here.

engine error on run

(Adding post docs-build)
running the recipe function (recipe_pruned.to_function()()) errors with ValueError: unrecognized engine h5netcdf must be one of: ['netcdf4', 'store', 'zarr']. You can see it here. I'm surprised this happens, since we run so many other recipes with the docs those libraries I would have thought have to be installed.

cc @cisaacstern

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@cisaacstern
Copy link
Member

Awesome! Thanks for getting this going, Rachel. More complete review to follow, but just wanted to note that we're tracking the pruning issue you encountered in #279 and IIUC Ryan is planning to fix this next week.

@rwegener2
Copy link
Member Author

With this most recent commit the notes from the PR description have been addressed:

chunksize and prunning
changed to inputs_per_chunk=2 to avoid the error for now. We can change this back to inputs_per_chunk=10 when #279 is complete

consolidated metadata warning
This is just a warning so its being ignored for now

logging output
A markdown comment was added to inform the user why the line is commented out

engine error on run
Fixed with #296

Status

I have no more todos on this PR and there are no outstanding questions. Ready for yes/no on merging.

@rabernat rabernat self-assigned this Feb 23, 2022
@@ -0,0 +1,1228 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Before we get here, I think we need a quick note about how to select a recipe class, or at least a link to the relevant docs (see #298).

It's okay to leave at as is for now, but we will need to remember to update here.


Reply via ReviewNB

@@ -0,0 +1,1228 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to work on improving our repr (see #299). No action needed here, but we will want to regenerate this once that is done.


Reply via ReviewNB

@@ -0,0 +1,1228 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

I honestly prefer to see the logging.


Reply via ReviewNB

@@ -0,0 +1,1228 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

We should suppress this warning. See #300.


Reply via ReviewNB

@rwegener2
Copy link
Member Author

rwegener2 commented Feb 23, 2022

@rabernat thanks for the review, most comments are addressed. 3 of the comments which I left unresolved I interpreted as future actions. The last comment I left was about logging. It is really nice while working but in my opinion it doesn't render very nicely on the webpage. It takes up a lot of room and the lack of color coding makes it hard to follow. Let me know if I've missed anything.

@cisaacstern
Copy link
Member

the lack of color coding makes it hard to follow

If rich is installed in the environment where the notebook is executed, the logs will be really colorful!

try:
from rich.logging import RichHandler
handler = RichHandler()
handler.setFormatter(logging.Formatter("%(message)s"))
except ImportError:
import sys
handler = logging.StreamHandler(stream=sys.stdout)
handler.setFormatter(logging.Formatter("%(name)s - %(levelname)s - %(message)s"))

screenshots in #287 (comment)

@cisaacstern
Copy link
Member

I would vote to include the 🎨rich logs in the tutorial.

If the only remaining concern is vertical space, it looks like MyST supports collapsable outputs via the hide-output tag:
https://myst-nb.readthedocs.io/en/latest/use/hiding.html#hide-code-cells

I believe this is an up-to-date explanation for how to add tags to cells:
https://jupyterbook.org/content/metadata.html#adding-tags-using-notebook-interfaces

So I think:

  • if each of the cells with logging output has the hide-output tag added to it; and
  • we add rich to the docs requirements.txt

then maybe MyST/Sphinx will just know what to do when the docs are built?

@rwegener2
Copy link
Member Author

I really like those ideas @cisaacstern , and thanks so much for finding instructions how to do it!

I just pushed a commit that shows what the tutorial would look like with the hide-output option. I added a line importing rich, but I think we will have to add it to the pip dependencies for the docs image and merge it before we'd be able to see what the effect on the docs. @rabernat if you agree with the use of rich I can put in a PR for that.

@rabernat
Copy link
Contributor

rabernat commented Feb 24, 2022

if you agree with the use of rich I can put in a PR for that

Yes, I agree with the use of rich. In order to get it into the doc environment, you just have to add it here

Scratch that

I think we will have to add it to the pip dependencies for the docs image and merge it before we'd be able to see what the effect on the docs

This is not correct. Since the notebook is being executed locally and then committed to the repo with its content already filled, its outputs are generated purely in your local environment. So you should just have to have rich installed locally. It should not have to be added to the dev environment in order to see the colorful logs.

I'm not sure why that isn't working here. Maybe a sphinx or myst-nb issue.


Since merging #295 it looks like there is a merge conflict on intro_tutorial_part1.ipynb here. @rwegener2 - do you know how to resolve that yourself? Merge conflicts in notebooks can be a nightmare. I recommend you use either an ours or theirs merge strategy to accept wholesale either the master branch version of the notebook or your new version. Otherwise it's very easy to mangle the notebook json. Ping me if you need help here.

@rabernat
Copy link
Contributor

Ah ok so it looks like the part 1 tutorial was simply removed in this PR. So it should be straightforward to resolve. You probably just want to revert commit 5b541f2. Hopefully it is as simple as git revert 5b541f29f6e13565e9d23edb94ab55d2f02584f8.

@rwegener2
Copy link
Member Author

That all makes sense @rabernat, thanks for explaining!

I'll fix the merge conflicts and re-run the docs with rich in my local docs build. I have an idea of where I may have messed up with the rich install

@rabernat
Copy link
Contributor

rabernat commented Feb 24, 2022

FWIW, we can still add rich to docs/requirements.txt. It is a light dependency. It won't affect the RTD build in this case, but it will ensure that people following the development documentation instructions have rich in their local dev environment.

@rwegener2
Copy link
Member Author

Okay so rich I can't get going. I am happy with the way that the logs look with the dropdown, though, so I would be fine to move on without rich and circle back at another point.

Merge conflicts are resolved. I think we are set here. Feel free to squash-merge or give me a 👍🏻 to do so.

@rabernat
Copy link
Contributor

Final suggestion: For the "code summary" at the end, put all the code into a single cell. That way it can easily be copy-pasted as a block.

Then feel free to self-merge.

@rwegener2 rwegener2 merged commit ef557f6 into pangeo-forge:master Feb 24, 2022
@rwegener2 rwegener2 deleted the intro_tutorial_part2 branch February 24, 2022 15:26
@rwegener2 rwegener2 added the documentation Improvements or additions to documentation label Mar 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants