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

[GHA] fix several codecov issues #912

Merged
merged 2 commits into from
Sep 18, 2023
Merged

[GHA] fix several codecov issues #912

merged 2 commits into from
Sep 18, 2023

Conversation

kiukchung
Copy link
Collaborator

@kiukchung kiukchung commented Sep 18, 2023

This PR does a few things:

  1. Excludes keras_core/legacy from coverage metrics since this module there for BC reasons
  2. Adds the test backend (jax, tensorflow, torch, numpy) to flags to allow tracking coverage metrics per backend. Note that each backend's coverage will be lower than the aggregate (e.g. keras_core will have higher coverage than keras_core-jax), so this flag should be used to track trends rather than impose thresholds.
  3. Moves coverage and pytest configuration from setup.cfg to pyproject.toml to keep all project configs in toml vs cfg (unfortuantely flake8 configs need to stay in setup.cfg since flake8 doesn't support pyproject.toml yet: pyproject.toml (PEP 518) support PyCQA/flake8#234)
  4. Adds fail_ci_if_error: true to codecov upload steps to fail the CI (and force the author to retry the workflow) on transient codecov upload failures of the form
     Unable to locate build via Github Actions API. Please upload with the Codecov repository upload token to resolve issue.', code='not_found
    
    This happens when we hit codecov API rate limits since we don't use CODECOV_TOKEN (see "Unable to locate build via Github Actions API" for the public repository codecov/feedback#126). We could circumvent the issue by adding the token as a secret but GH secrets are not visible from forks, implying that contributors will not be able to get coverage diffs for their PRs. To allow this, the only option seems to be to hardcode the token (which seems ok since the token can be setup with limited permissions) but hardcoding tokens is just sketchy...

…nd to codecov flags, move project config to pyproject.toml
@kiukchung
Copy link
Collaborator Author

kiukchung commented Sep 18, 2023

will graduate from draft commit after I validate GHA and check codecov reports
codecov metrics + flags look good

@codecov
Copy link

codecov bot commented Sep 18, 2023

Codecov Report

Patch coverage has no change and project coverage change: +6.79% 🎉

Comparison is base (a465816) 76.82% compared to head (742e71c) 83.62%.
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     codecov/codecov-action#912      +/-   ##
==========================================
+ Coverage   76.82%   83.62%   +6.79%     
==========================================
  Files         329      318      -11     
  Lines       31427    28390    -3037     
  Branches     6112     5409     -703     
==========================================
- Hits        24144    23740     -404     
+ Misses       5719     3150    -2569     
+ Partials     1564     1500      -64     
Flag Coverage Δ
keras_core 83.51% <ø> (+6.78%) ⬆️
keras_core-jax 67.58% <ø> (?)
keras_core-numpy 60.73% <ø> (?)
keras_core-tensorflow 67.07% <ø> (?)
keras_core-torch 69.42% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 13 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kiukchung kiukchung changed the title [ci/cd] exclude keras_core/legacy from coverage.run|report and add ba… [GHA] fix several codecov issues Sep 18, 2023
@kiukchung kiukchung marked this pull request as ready for review September 18, 2023 22:14
Copy link
Member

@fchollet fchollet left a comment

Choose a reason for hiding this comment

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

LGTM -- thank you!

@fchollet fchollet merged commit f38e2e6 into main Sep 18, 2023
10 checks passed
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.

2 participants