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

Add GitHub Actions Workflows. #471

Merged
merged 29 commits into from
Jan 7, 2023
Merged

Add GitHub Actions Workflows. #471

merged 29 commits into from
Jan 7, 2023

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Dec 22, 2022

This PR adds GitHub Actions workflows to cucim.

Task list

Coverage required for this PR:

  • C++ build
  • C++ tests (There are no C++ tests.)
  • Python build
  • Python tests
    • Some tests have been marked as xfail on ARM following discussion with @jakirkham and @grlee77. Those will be addressed in follow-up work.
  • Style checks

@codecov-commenter
Copy link

codecov-commenter commented Dec 22, 2022

Codecov Report

❗ No coverage uploaded for pull request base (branch-23.02@dbbeaf9). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@               Coverage Diff               @@
##             branch-23.02     #471   +/-   ##
===============================================
  Coverage                ?   92.94%           
===============================================
  Files                   ?      130           
  Lines                   ?     9773           
  Branches                ?        0           
===============================================
  Hits                    ?     9084           
  Misses                  ?      689           
  Partials                ?        0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@ajschmidt8 ajschmidt8 added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Jan 1, 2023
@@ -0,0 +1,62 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

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

@bdice, I see this test_notebooks.sh file, but I don't see a corresponding job in pr.yaml.

Also, it doesn't seem that cucim notebooks are currently tested in Jenkins PRs.

Therefore, should we punt notebooks testing for cucim for a separate PR to prevent scope creep?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was going to make a quick attempt at running notebooks and defer to a later PR if it does not work.

In general, my strategy for all the tasks we want to do in this release is to try things aggressively, and reduce scope if an “optional” task fails after some small, fixed amount of effort. Some bets pay off, others do not, but I try to keep the downside loss (budget of effort for non-essential goals) small.

@ajschmidt8
Copy link
Member

Here's a summary of the latest pytest failures:

  • on amd64
    • it seems like all of the amd64 tests only have a single failure (1,2,3) that says ImportError: libGL.so.1: cannot open shared object file: No such file or directory. I can't seem to figure out where this .so is available in our Jenkins CI images, so I'm not sure how to fix it for GHAs.
  • on arm64
    • the arm64 test failures (link) have the same .so error as amd64, plus a few others. Just a reminder, we don't test arm64 in PRs on Jenkins, so these failures could be legit

@bdice
Copy link
Contributor Author

bdice commented Jan 2, 2023

I think the libGL.so.1 errors are from attempting to run code that requires a display (or virtual GL output device?) on a headless machine without GL libraries.

On my WSL Ubuntu 22.04 system, I have /usr/lib/x86_64-linux-gnu/libGL.so.1 which is provided by the apt package libgl1. I am not sure if that's sufficient here. We might be able to do something like export an environment variable to tell the package to run in "headless" mode. I see there are some channels like fastai offering a package named opencv-python-headless, and a PyPI package: https://pypi.org/project/opencv-python-headless/ but no conda-forge package labeled "headless."

Some related StackOverflow answers are here: https://stackoverflow.com/questions/55313610/importerror-libgl-so-1-cannot-open-shared-object-file-no-such-file-or-directo

@bdice
Copy link
Contributor Author

bdice commented Jan 3, 2023

I think we have a few potential courses of action:

  • Install libgl1 in Ubuntu images and mesa-libGL in CentOS/Rocky images. I am convinced that this is necessary to use the conda-forge packages because of this comment
  • Use the fastai::opencv-python-headless package and skip OpenCV tests on aarch64 (package not available for aarch64)
  • Try the PyPI opencv-python-headless by adding it as a pip dependency in the conda environment -- not sure if this would work but it might be worth trying
  • Skip these tests entirely (probably not a sufficient solution)

@bdice
Copy link
Contributor Author

bdice commented Jan 3, 2023

@ajschmidt8 I implemented the first proposed solution here: rapidsai/ci-imgs#31

If you're satisfied with that, we can update the images and retry this PR.

edit: We decided against that approach and used a "headless" version of opencv from pip in 6045729. See rapidsai/ci-imgs#31 (comment)

@bdice bdice changed the title Add GitHub Actions Workflows. [skip gpuci] Add GitHub Actions Workflows. Jan 4, 2023
@bdice bdice marked this pull request as ready for review January 6, 2023 04:42
@bdice bdice requested review from a team as code owners January 6, 2023 04:42
Comment on lines +18 to +20
# TODO: Some tests fail unexpectedly on ARM.
ON_AARCH64 = platform.machine() == "aarch64"
ON_AARCH64_REASON = "TODO: Test fails unexpectedly on ARM."
Copy link
Member

@jakirkham jakirkham Jan 6, 2023

Choose a reason for hiding this comment

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

We decided to skip these after discussing with @grlee77 offline. Noting this here for clarity. Also for reference in a follow up issue ( #473 )

@jakirkham jakirkham mentioned this pull request Jan 6, 2023
5 tasks
@ajschmidt8
Copy link
Member

/merge

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 all! 🙏

Sorry spotted a couple things below that I had questions on. Just want to make sure I'm not missing something

@@ -1,20 +0,0 @@
# To install the git pre-commit hook run:
Copy link
Member

Choose a reason for hiding this comment

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

Was this meant to be dropped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This shouldn't exist as far as I understand. The only config that should matter is in the repo root.

Copy link
Member

Choose a reason for hiding this comment

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

Interestingly it seems we are including this one in MANIFEST.in though

In any event, agree we should only have this in one place. If we stick to dropping this one, the MANIFEST.in needs an update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Removed from MANIFEST.in in 93c2c1e.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks Bradley! 🙏

Given the file is still here (just in another location), it is not really being dropped. If it turns out we need to move it for whatever reason, we can do that in a follow up.

conda/environments/all_cuda-115_arch-x86_64.yaml Outdated Show resolved Hide resolved
dependencies.yaml Outdated Show resolved Hide resolved
@jakirkham
Copy link
Member

/merge

@rapids-bot rapids-bot bot merged commit a2c19e9 into rapidsai:branch-23.02 Jan 7, 2023
@bdice bdice mentioned this pull request Jan 10, 2023
@jakirkham jakirkham added this to the v23.02.00 milestone Feb 1, 2023
rapids-bot bot pushed a commit that referenced this pull request Apr 5, 2023
This PR re-enables pre-commit checks for normalizing whitespace (trimming trailing spaces) that I disabled in #471.

These pre-commit hooks existed prior to the GitHub Actions migration but they were not being enforced by CI style checks. I disabled them in #471 because enforcing them would have made the PR diff large.

The primary change is in `.pre-commit-config.yaml`, and everything else was automatically fixed by `pre-commit`.
```
# TODO: re-enable this after GitHub Actions migration, it currently makes a
# large number of changes that shouldn't go in the GitHub Actions PR.
#- repo: https://github.com/pre-commit/pre-commit-hooks
#  rev: v4.4.0
#  hooks:
#    - id: trailing-whitespace
#    - id: end-of-file-fixer
#    - id: debug-statements
```

Authors:
  - Bradley Dice (https://github.com/bdice)
  - https://github.com/jakirkham

Approvers:
  - Gregory Lee (https://github.com/grlee77)
  - Ray Douglass (https://github.com/raydouglass)
  - https://github.com/jakirkham

URL: #474
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants