Actions for Generating and Checking Jupyter Notebooks

[lettlini]

• Have you followed our guidelines in CONTRIBUTING.md?
• Have you self-reviewed your code and corrected any misspellings?
• Have you written documentation that is easy to understand?
• Have you written descriptive commit messages?
• Have you added NumPy docstrings for newly added functions?
• Have you formatted your code using black?
• If you have introduced a new functionality, have you added adequate unit tests?
• Have all tests passed in your local clone?
• If you have introduced a new functionality, have you added an example notebook?
• Have you kept your pull request small and limited so that it is easy to review?
• Have the newest changes from this branch been merged?

This pull request adds actions for

1. Generating all jupyter notebooks in examples/ (pushing generated notebooks)
2. Checking if all jupyter notebooks in examples/ run without any errors

[codecov]

Codecov Report

Patch coverage has no change and project coverage change: +7.32 🎉

Comparison is base (f4a3e5a) 47.41% compared to head (a674251) 54.73%.

Additional details and impacted files

@@              Coverage Diff              @@
##           RC_v1.5.0     #258      +/-   ##
=============================================
+ Coverage      47.41%   54.73%   +7.32%     
=============================================
  Files             14       15       +1     
  Lines           2708     3144     +436     
=============================================
+ Hits            1284     1721     +437     
+ Misses          1424     1423       -1     

Flag	Coverage Δ
unittests	54.73% <ø> (+7.32%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[lettlini]

I have adjusted the trigger events according to the suggestions of @w-k-jones.
It appears that the Jupyter Notebook CI action fails because there is an error in one of the documentation notebooks.
I am not quiet sure if it is desirable to run the action on the notebooks in doc/. If someone could clarify this, I am happy to update this action's definition.

[freemansw1]

ha, that's amazing. Your CI discovers a bug before even being implemented. I love it. We should probably fix that.

I think the notebooks in doc should also be covered by this CI.

[w-k-jones]

I agree, running on the notebooks on doc is ideal, and it's arguably even more important that those ones work!

[lettlini]

Alright, thank you both for the clarification! The "Generate Jupyter Notebooks" action currently does not run on the notebooks in doc/. Should I change this?

[w-k-jones]

I think that I would be a good idea to make it run on the notebooks in doc/ as well, to make sure that they are always up to date for the current version

[lettlini]

I agree with you @w-k-jones, I have updated the path in the corresponding find command accordingly.

[freemansw1]

@lettlini Are you happy for this to be reviewed and moved into an active PR? Given that your new action will fail (due to bugs in the notebooks), we should probably fix the notebook bugs before merging. That's probably a new issue and PR, and should be done before releasing v1.5.0.

[lettlini]

I think this PR is ready for being reviewed.

I agree with you @freemansw1, we should fix the errors in the notebooks before merging this PR.

[w-k-jones]

Great, awesome work @lettlini ! Do you have time to fix the notebook bugs or should someone else pick that up?

[lettlini]

I think I already found the problem. I am going to create a PR shortly

[lettlini]

It seems to have been just one notebook that was causing errors. I think I fixed the problem with #269.

[freemansw1]

Okay, with #269 merged, @lettlini if you merge the current changes, the CI should pass and we can get this in. Thanks so much for your hard work on this!

[lettlini]

It seems that pytables has been removed from the requierements file in RC_v1.5.0. However, the notebooks still need it to run. There are two possible solutions:

1. Add it back to the requirements.txt file
2. Adding an explicit install instruction in the action definition (i.e. conda install ... pytables)

What do you think is the best course of action?

[freemansw1]

It seems that pytables has been removed from the requierements file in RC_v1.5.0. However, the notebooks still need it to run. There are two possible solutions:

1. Add it back to the requirements.txt file
2. Adding an explicit install instruction in the action definition (i.e. conda install ... pytables)

What do you think is the best course of action?

It's funny; we had a whole conversation about this. I'm inclined for option 2, although longer-term, I'm inclined to have pandas save to non-HDF formats (well, longer term, this is irrelevant due to the xarray transition)

[w-k-jones]

I think for now we should be keep pytables as a dependency simply so that the notebooks work out of the box. It will be nice when the xarray transition makes this unnecessary!

[w-k-jones]

Looks good overall. My only comments are regarding whether we need a specific version of python, as this may lead to a bit more overhead in future. It's very minor, so not a blocking issue

[freemansw1]

I think for now we should be keep pytables as a dependency simply so that the notebooks work out of the box. It will be nice when the xarray transition makes this unnecessary!

I'm not opposed to this, but we did explicitly remove it (#119 ) and I'm hesitant to then re-add it.

[freemansw1]

This is excellent work, and I'm happy for this to be merged once @w-k-jones's last comments around version and pytables are resolved.

[w-k-jones]

Great! Thanks for the fixes @lettlini , I think this is now ready to merge. The more general pytables issue can be left for another day, as it's been handled within the scope of this PR

[freemansw1]

@lettlini I think we're ready to merge if you are.

[lettlini]

I'm happy for this to be merged.

[w-k-jones]

Ok, potential problem here. When running the workflow after merging this the following error occurred on the Generate-Notebooks workflow:

[RC_v1.5.0 2ca348f] Generated Notebooks
 8 files changed, 90340 insertions(+), 292 deletions(-)
remote: Permission to tobac-project/tobac.git denied to github-actions[bot].
fatal: unable to access 'https://github.com/tobac-project/tobac/': The requested URL returned error: 403
Error: Process completed with exit code 128.

The repository has read and write permissions enabled for workflow actions, so I'm not sure what is going on here. Perhaps the write protections are preventing the commit to the RC branch?

[lettlini]

Sorry this is causing problems! I think you are right with the assumption that this issue is caused by the branch protection rules. I simply overlooked this.
I will look for a fix first thing tomorrow!

[freemansw1]

https://github.com/orgs/community/discussions/13836 may be a good resource here. Happy to edit the branch protections as needed for this use case.

[lettlini]

Thank you @freemansw1 for the reference. After looking at this problem again I am unfortunately convinced that there is no easy solution to this problem.

I think the branch protection rules are a very good idea and should definitely be kept. So I would like to suggest that we disable this action for now and look for another way the jupyter notebook generation can be automated.

Since the check_notebooks action is not affected we should probably keep it.

[w-k-jones]

There is a potential workaround; we can exclude specific accounts from the protection rules. Would it be possible to make an action account for tobac and use that to submit the generated notebooks?

[lettlini]

According to the link posted by @freemansw1 that could work. I'm just not really convinced that this is a good solution.

[freemansw1]

@lettlini I'd like to understand more about your reasoning here... it seems to me like making an app under our organization with these special permissions wouldn't be too bad.

[lettlini]

If I understand this correctly, @w-k-jones is proposing the following:

1. create a new github user account
2. allow this account to push to protected branches
3. create a personal access token for this user
4. store this personal access token as a secret in the main repository
5. use this token in the github action to push to protected branches

(Summarized from https://github.com/orgs/community/discussions/13836#discussion-3973425)

Is this what you meant @w-k-jones?

[freemansw1]

@lettlini I was thinking of making the action an app, rather than tied to a user, as is done here: https://github.com/orgs/community/discussions/13836#discussioncomment-4851440 . We should be able to make an app under the tobac-project organization and give the CI that access token without it being a new user account, I think? This is unclear to me.

[freemansw1]

I'm going to open an issue on this. I think resolving this one way or another needs to be a blocking issue before v1.5 is released. I'd like to resolve it such that we generate the notebooks through CI, but if that's not possible, we will need to manually regenerate them before release.