Skip to content

Add GitHub Actions Workflows. - #471

Merged
rapids-bot[bot] merged 29 commits into
rapidsai:branch-23.02from
bdice:gha
Jan 7, 2023
Merged

rapids-bot[bot] merged 29 commits into
rapidsai:branch-23.02from
bdice:gha

Conversation

@bdice

@bdice bdice commented Dec 22, 2022 •

Copy link
Copy Markdown
Contributor

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

codecov-commenter commented Dec 22, 2022 •

Copy link
Copy Markdown

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
Comment thread ci/test_notebooks.sh Outdated
@@ -0,0 +1,62 @@
#!/bin/bash

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

bdice commented Jan 2, 2023 •

Copy link
Copy Markdown
Contributor Author

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

bdice commented Jan 3, 2023

Copy link
Copy Markdown
Contributor Author

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

bdice commented Jan 3, 2023 •

Copy link
Copy Markdown
Contributor Author

@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."

@jakirkham jakirkham Jan 6, 2023 •

Copy link
Copy Markdown
Member

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
Copy Markdown
Member

/merge

@jakirkham jakirkham left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread conda/environments/all_cuda-115_arch-x86_64.yaml
Comment thread dependencies.yaml Outdated
@jakirkham

Copy link
Copy Markdown
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