-
Notifications
You must be signed in to change notification settings - Fork 45
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
ipython notebook test #330
Conversation
* fix cdip * D3D TRTS eg fix * fix pylint error * pylint * fix too many branches
* fix cdip * D3D TRTS eg fix * fix pylint error * pylint * fix too many branches * fix map and Tp * folium fix & Tp using pandas * up to python 3.10 * py version as string * fix variable check for new xarray * undo changes
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.
Thanks @ssolson
This all looks good to me. All the notebook tests are all running. I have a few clarifying questions to understand the Actions set-up better, but no issues with the approach.
run: | | ||
if [[ "${{ matrix.notebook }}" == "examples/metocean_example.ipynb" || "${{ matrix.notebook }}" == "examples/WPTO_hindcast_example.ipynb" ]]; then | ||
if [[ "${{ needs.check-changes.outputs.should-run-hindcast }}" == 'true' ]]; then | ||
jupyter nbconvert --to notebook --execute --inplace --ExecutePreprocessor.timeout=${{ matrix.timeout }} "${{ matrix.notebook }}" |
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.
Will "ExecutePreprocessor" fail each notebook test if it hits the timeout?
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.
Yes each notebook in the repo has a custom timeout specified. It will fail if you go overtime. You can see some of these failures on previous commits such as the commit before "Increase ADCP timeout".
always() && | ||
( |
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.
"always() &&" seems redundant
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.
I agree, however, I have become accustomed to this syntax in actions and you will find it throughout our tests.
There is discussion here agreeing that you should not need to but that you do:
https://github.com/orgs/community/discussions/25789#discussioncomment-3249273
- name: Generate matrix | ||
id: set-matrix | ||
run: | | ||
matrix_json=$(python .github/workflows/generate_notebook_matrix.py) |
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.
What is the purpose of the matrix_json
variable here? Isn't matrix
the only variable from generate_notebook_matrix.py
that is used? Similarly, where is the matrix
key "include" being used?
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.
In bash here we are setting a variable matrix_json
On the next line we echo that variable to send it as std_out via ">>" to the variable matrix
.
So the matrix_json
variable is used in the echo command
notebooks.append(os.path.join(root, file)) | ||
|
||
# Generate the matrix configuration | ||
matrix = {"include": []} |
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 the key "include" have to do with how Actions creates a testing matrix from the test variable?
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.
Yes.
The include key is a dummy key/ catch all for Actions because you can only have a single variable json at the top level.
So its called include to include all the keys I want to use.
This PR adds a Github action to test the example notebooks as part of or CD pipeline. Additionally a timeout is added to which notebooks will fail if they exceed the given time.
This PR adds a Github action to test the example notebooks as part of or CD pipeline. Additionally a timeout is added to which notebooks will fail if they exceed the given time.
To accomplish this:
generate_notebook_matrix.py
is a new script called by the above jobAdditionally this drops testing of Python 3.8 and 3.9 which will be required by #333.
Closes #329