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

Codecov and reporter for CI #167

Merged
merged 4 commits into from Mar 16, 2022
Merged

Conversation

EricGustin
Copy link
Contributor

This PR adds running the coverage tests to the GitHub Actions CI. In addition to running the coverage tests, the resulting coverage report is uploaded to codecov, and a PR bot will report any coverage changes that a given PR causes. We also now have a codecov badge for our README.md.

@codecov-commenter
Copy link

codecov-commenter commented Mar 7, 2022

Codecov Report

❗ No coverage uploaded for pull request base (develop@dba773b). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop     #167   +/-   ##
==========================================
  Coverage           ?   80.51%           
==========================================
  Files              ?       57           
  Lines              ?     2910           
  Branches           ?        0           
==========================================
  Hits               ?     2343           
  Misses             ?      567           
  Partials           ?        0           

@ben-albrecht
Copy link
Contributor

These changes look good to me. Any idea why the badge on your branch's README shows codecov status as 'unknown'?

@EricGustin
Copy link
Contributor Author

EricGustin commented Mar 9, 2022

Thanks @ben-albrecht
So the reason why it says unknown on my branch is because the badge is linked to CrayLabs' develop branch, so once these coverage tests are ran on develop (which wouldn't happen until after merge), then the correct coverage % should be displayed. A similar situation occurred when the coverage badge was added to SmartRedis.

@Spartee
Copy link
Contributor

Spartee commented Mar 10, 2022

@EricGustin, it seems you and @al-rigazzi have figured out the bug in the one ubuntu version? the decision for now is to skip torch there and recommend users go with RAI 1.2.5 if they are block on this correct? Otherwise is this ready to go in?

@al-rigazzi
Copy link
Collaborator

al-rigazzi commented Mar 10, 2022

Yeah @Spartee, it is a very specific PyTorch 1.7.1 + Python 3.9 (+ Ubuntu, possibly) problem. See e.g. this issue or one of the many other similar ones.

This was then solved in PyTorch 1.8

So yes, I'll basically repeat what you wrote, as it would be my suggestion (but @EricGustin may have changed his mind, so he'll have the last word):

  • do not install torch in that CI specific matrix entry (i.e. RAI 1.2.3, Python 3.9, Ubuntu)
  • start pushing users to RAI 1.2.5 (we have clues that it is more stable on other systems too)

@EricGustin
Copy link
Contributor Author

As Al said there is a bug with Ubuntu/Python3.9/PyTorch1.7.1 which has been documented in several tickets like the one linked above. @Spartee and @al-rigazzi I've configured the Yaml file such that torch is not installed for the matrix combination Ubuntu20.04/Python3.9/RedisAI1.2.3. CI is now passing, good to merge?

Copy link
Contributor

@Spartee Spartee left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@al-rigazzi al-rigazzi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for fixing the Pytorch/Python issue

@EricGustin EricGustin merged commit 187654d into CrayLabs:develop Mar 16, 2022
@EricGustin EricGustin deleted the ss-codecov branch March 16, 2022 18:31
@EricGustin EricGustin restored the ss-codecov branch March 16, 2022 18:48
@EricGustin EricGustin deleted the ss-codecov branch March 16, 2022 18:49
al-rigazzi added a commit that referenced this pull request Mar 30, 2022
Adds new tutorial for data loaders and online training, which
is rendered as a Notebook in documentation and can
be run on a laptop.

[ committed by @al-rigazzi ]
[ reviewed by @Spartee ]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants