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

ensure notebook-testing script fails when notebooks fail #1424

Merged
merged 10 commits into from
Aug 13, 2024

Conversation

jameslamb
Copy link
Member

@jameslamb jameslamb commented Aug 1, 2024

Description

The notebook-testing CI job in this repo does not actually cause a loud CI failure if any errors are detected in notebooks. That's because of a bash mistake I made in #1407... that PR moved notebook-checking into a function, but didn't add a set -E to be sure errors from inside that function were appropriately trapped.

This PR fixes that:

  • ensures that notebook failures actually cause CI failures
  • fixes 2 typos in nyc_taxi_years_correlation.ipynb code

Context: #1422 (review)

Notes for Reviewers

How I tested this

Originally did not skip any notebooks. Saw the failures in one of the notebooks cause an actual merge-blocking CI failure.

Build link: https://github.com/rapidsai/cuspatial/actions/runs/10199784404/job/28219162698?pr=1424

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@jameslamb jameslamb added the 2 - In Progress Currenty a work in progress label Aug 1, 2024
@github-actions github-actions bot added the ci label Aug 1, 2024
@jameslamb jameslamb added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Aug 1, 2024
ci/test_notebooks.sh Outdated Show resolved Hide resolved
@jameslamb jameslamb changed the title WIP: ensure notebook-testing script fails when notebooks fail ensure notebook-testing script fails when notebooks fail Aug 1, 2024
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@github-actions github-actions bot added the Python Related to Python code label Aug 2, 2024
@jameslamb jameslamb self-assigned this Aug 2, 2024
@jameslamb
Copy link
Member Author

This is ready for review. The failing unit test jobs are unrelated... they're the result of #1427, which will hopefully be fixed by @mroeschke 's PR: #1428

@jameslamb jameslamb marked this pull request as ready for review August 2, 2024 21:58
@jameslamb jameslamb requested a review from a team as a code owner August 2, 2024 21:58
@jameslamb jameslamb added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currenty a work in progress labels Aug 2, 2024
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks James! 🙏

Looks good. Had one minor suggestion

@jakirkham
Copy link
Member

There were a couple CI jobs that appear to have had issues starting up (this is well before anything cuSpatial related AFAICT)

Have gone ahead and restarted those failing jobs. Hopefully that clears out the issue

@jakirkham
Copy link
Member

/merge

@jameslamb
Copy link
Member Author

Thanks @jakirkham !

I think we'll need #1434 to be finished to unblock CI in this repo.

@jameslamb
Copy link
Member Author

One the notebooks is failing with what looks like a new cudf-compatibility issue. Wrote that up here: #1437

@jakirkham
Copy link
Member

Going to covert to draft for clarity while those other blockers get cleared out

@jakirkham jakirkham marked this pull request as draft August 12, 2024 23:04
@harrism
Copy link
Member

harrism commented Aug 13, 2024

I think draft should be reserved for PRs unready for review. This PR is already reviewed, but can't be merged. I suggest using the DO NOT MERGE label, which does what it says on the tin.

@jameslamb jameslamb added 5 - DO NOT MERGE Hold off on merging; see PR for details and removed 3 - Ready for Review Ready for review by team labels Aug 13, 2024
@jameslamb jameslamb marked this pull request as ready for review August 13, 2024 13:14
@jameslamb
Copy link
Member Author

I think draft should be reserved for PRs unready for review.

No problem, I've moved this back to Ready for Review and added the DO NOT MERGE label.

@jameslamb
Copy link
Member Author

Hopefully CI here will pass once #1438 is merged.

@jameslamb jameslamb removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label Aug 13, 2024
@rapids-bot rapids-bot bot merged commit 82f321b into rapidsai:branch-24.10 Aug 13, 2024
57 checks passed
@jameslamb jameslamb deleted the ci/notebook-script branch August 13, 2024 23:42
@jameslamb
Copy link
Member Author

Thanks so much for all the help, everyone! Sorry this took a couple iterations, but I think this testing is in a better state now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Related to Python code
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

5 participants