Make codecov pull requests informational only #269

jagerber48 · 2024-11-13T06:18:22Z

Closes Fix various code coverage issues #270
Executed pre-commit run --all-files with no errors
The change is fully covered by automated unit tests
Documented in docs/ as appropriate
Added an entry to the CHANGES file

This PR hopes to resolve a few issues with uncertainty package interaction with codecov.io.

documentation building, readthedocs config #203 introduced .codecov.yaml. Unfortunately, I think this was an invalid configuration. Specifically because the comment section is supposed to be a top-level section, not nested under the codecov section. This means that since that PR the yaml configuration has NOT been used for our codecov updates. I think this explains why we're seeing codecov failures despite best attempts to suppress them.
See for example Change how numpy behaves as an optional import #268 where a codecov failure is creating an X when we really just want the PR to go through. This PR will (attempt to) make it so that codecov coverage is reported, but will never result in a failed coverage run. This way reviewers can see if patch coverage is < 100% or if overall coverage drops due to a PR. If reviewers deem a lack or drop of coverage to be significant they can block PR approval. Otherwise, for minor coverage drops, the coverage can be easily ignored. It does so using the informational flag in the .codecov.yaml file.
It moves the API token back to using the github secret rather than exposing it in the .codecov.yaml file.

codecov · 2024-11-13T06:36:31Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.46%. Comparing base (9a4c275) to head (a3359d2).
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #269   +/-   ##
=======================================
  Coverage   96.46%   96.46%           
=======================================
  Files          16       16           
  Lines        1898     1898           
=======================================
  Hits         1831     1831           
  Misses         67       67

Flag	Coverage Δ
macos-latest-3.10	`95.83% <ø> (?)`
macos-latest-3.11	`95.83% <ø> (?)`
macos-latest-3.12	`95.83% <ø> (?)`
macos-latest-3.8	`95.83% <ø> (?)`
macos-latest-3.9	`95.83% <ø> (?)`
no-numpy	`74.18% <ø> (?)`
ubuntu-latest-3.10	`95.83% <ø> (?)`
ubuntu-latest-3.11	`95.83% <ø> (?)`
ubuntu-latest-3.12	`95.83% <ø> (?)`
ubuntu-latest-3.8	`95.83% <ø> (?)`
ubuntu-latest-3.9	`95.83% <ø> (?)`
windows-latest-3.10	`95.83% <ø> (?)`
windows-latest-3.11	`95.83% <ø> (?)`
windows-latest-3.12	`95.83% <ø> (?)`
windows-latest-3.8	`95.83% <ø> (?)`
windows-latest-3.9	`95.83% <ø> (?)`

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

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

…report to codecov.io

jagerber48 · 2024-11-14T13:38:45Z

This PR is going very badly. I've discovered

If I include the codecov token explicitly in the source code (something which we shouldn't do because I think it's a security risk, I think that token is something like a password) then all the 16 codecov uploads work in github actions but I don't see any uploads appear on codecov.io.
If the codecov token is kept secret (as it should be) then all codecov upload jobs, or all but one, will fail in github actions. Maybe the one that succeeds will appear on codecov.io.

I think the issue is related to this being a PR from a forked branch. I'm not sure what needs to be done about it though.

wshanks

Although all the checks are green, it seems like something is still wrong with Codecov. When I look at the Upload coverage step of 3.8 ubuntu-latest, I see:

error - 2024-11-14 13:33:38,751 -- Upload failed: [{"input":{"branch":"jagerber48:feature/code_cov_informational","commit_id":"4026faaab4ee6d4c6163e872defee173ec28988d","repo_id":18933979,"repo_name":"uncertainties"},"loc":["id"],"msg":"Field required","type":"missing","url":"https://errors.pydantic.dev/2.4/v/missing"}]

Looking at the Codecov page, it seems to have only one report instead of one per job in the matrix. I am not sure what report that is. It seems like all uploads failed on the latest run.

wshanks · 2024-11-14T13:49:47Z

.github/workflows/python-package.yml

@@ -53,14 +53,15 @@ jobs:
        python -m pip install .[test]
    - name: Test source code and docs
      run:
-        cd tests;


Do you have to delete cd tests;? With the default configuration, running pytest puts the working directory on the Python import path and so import uncertainties uses the files in the repo instead of the ones installed in site-packages. If there is an error in the packaging configuration such that a necessary file is not included in the package, the tests will still pass if they test with the files in the repo instead of the with the installed package. There are other ways to change pytest's behavior but just doing cd tests is a simple way to make sure the package is tested.

I need to understand this more. I think I would prefer to configure the pytest runs more explicitly rather than relying on monkeying around with cd and the implicit "path".

That is fine but we can do that in a separate PR? For a long time, the standard way to do this was to use a src/ directory at the top level of the repo with the package inside of it. More recently pytest added some alternative ways of importing test modules instead of putting the working directory on the Python path. They come with some caveats though that we would have to assess. They are documented here:

https://docs.pytest.org/en/stable/explanation/pythonpath.html

Yes, happy to discuss/do this in a separate PR. I'll pull that change out of this PR when I get a chance.

wshanks · 2024-11-14T14:00:16Z

.github/workflows/python-package.yml

      with:
-        token: ${{ secrets.CODECOV_TOKEN }}
-        slug: lmfit/uncertainties
+        file: ./coverage.xml


Could we add a name entry here as well? If we set the name based on the Python version and OS, would that let us tell which report was from which run? Right now all the reports for PR have the same name on the Codecov page so it is hard to tell which is which. This mainly matters for distinguishing the no-numpy run from everything else.

I think tags are what we're looking for here. Yes, we can do that. It's been my plan to add tags, but I wanted to get the more basic configuration working. I doubt adding tags will fix the issues we're seeing, but maybe I'm wrong. Maybe the tags will help.

Maybe we can do names also though.

jagerber48 · 2024-11-14T14:27:18Z

Although all the checks are green, it seems like something is still wrong with Codecov. When I look at the Upload coverage step of 3.8 ubuntu-latest, I see:

error - 2024-11-14 13:33:38,751 -- Upload failed: [{"input":{"branch":"jagerber48:feature/code_cov_informational","commit_id":"4026faaab4ee6d4c6163e872defee173ec28988d","repo_id":18933979,"repo_name":"uncertainties"},"loc":["id"],"msg":"Field required","type":"missing","url":"https://errors.pydantic.dev/2.4/v/missing"}]

Looking at the Codecov page, it seems to have only one report instead of one per job in the matrix. I am not sure what report that is. It seems like all uploads failed on the latest run.

Yes, see the issue I linked to in codecov feedback. I'm not sure what's going on here. As I said in my other comment, this error goes away if I include the token explicitly, but even though the github action job succeeds, the coverage reports don't appear on codecov.io.

jagerber48 · 2024-11-14T17:22:43Z

It looks like many of the current issues are stemming from the codecov.io server side as per the linked issue. Once those are resolved I think the main push of this PR --- improving and cleaning up codecov config --- can be resolved.

jagerber48 · 2024-11-14T19:53:38Z

Ok, seems like codecov/feedback#577 is resolved and our codecov reports are getting uploaded. Will tie up loose ends on this then it will be ready for review.

However, it is troubling the codecov can have strange issues on their end that bothers our CI. In any case, making the code coverage informational only means that these issues don't need slow down our progress if we're happy to move forward even if there are minor coverage issues.

jagerber48 · 2024-11-15T03:00:28Z

I've expanded the scope of this PR to include codecov tags. @wshanks any objections? I can move that change into a new PR if we prefer.

jagerber48 · 2024-11-15T03:24:49Z

Some more changes:

I regenerated the codecov token so that it is not the old token that has been revealed in historic source code
I pinned the test_without_numpy to python 3.12. This clears an error that was happening during the build due to the matrix of python versions not being valid during the no_numpy build.
I added a flag to each upload

On a previous commit I would put two flags on each with numpy codecov upload. e.g. windows and 3.12. codecov gives a warning that you should only have one flag per upload. I think the issue is that it is kind of strange to have the "windows" flag show the union of the coverage over all python versions. So I heeded codecov's warning and changed the double tags into compound single flags, like windows-3.12. This means there are more flags, but I think it means the reporting of coverage over time will be more accurate. Note however that it should be very very rare for us to see an coverage differences between python versions or operating systems.

jagerber48 · 2024-11-15T03:42:27Z

This PR is ready for review.

After this I'll transition back to working on source code instead of CI. However, it is on my radar, with low priority, to make some minor improvements to the CI configuration. Mostly just to make the github actions yaml file even simpler and easier to follow. This would include the cd tests change discussed above.

newville

Great! Thanks for working this out. I think this is the right approach.
+1 on merging.

jagerber48 added 2 commits November 12, 2024 23:00

prune .codecov.yaml

23a4ce9

try v4

b504b62

jagerber48 added 9 commits November 12, 2024 23:56

try changing how coverage is generated to see if that helps coverage …

6f1770f

…report to codecov.io

more changes

6247a5d

try no slug

f30640a

do token differently..

4467033

try file again

d3b7358

different github action version

415c4bc

do versions in right spot

332898b

explicit token

c7ae34b

back to secret token

4026faa

jagerber48 mentioned this pull request Nov 14, 2024

GitHub Actions upload failure: Failed to properly create report (status code 202, "msg": "Field required") codecov/feedback#577

Closed

wshanks reviewed Nov 14, 2024

View reviewed changes

jagerber48 added 3 commits November 14, 2024 19:51

revert pytest changes

a2634b7

codecov can find the coverage file on its own

70f1ec0

add codecov tags

4b91ac9

jagerber48 added 2 commits November 14, 2024 20:07

try dash instead of underscore

d9726ed

double flag to compound flag

8142545

jagerber48 added 3 commits November 14, 2024 20:26

test adding name for better reading of uploads

ce0c664

actually the name is redundant

8dc7bac

changelog

a3359d2

newville reviewed Nov 15, 2024

View reviewed changes

newville approved these changes Nov 15, 2024

View reviewed changes

jagerber48 merged commit c3da2a4 into lmfit:master Nov 15, 2024
21 checks passed

jagerber48 deleted the feature/code_cov_informational branch November 15, 2024 15:31

jagerber48 mentioned this pull request Nov 15, 2024

Change how numpy behaves as an optional import #268

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make codecov pull requests informational only #269

Make codecov pull requests informational only #269

jagerber48 commented Nov 13, 2024 •

edited

Loading

codecov bot commented Nov 13, 2024 •

edited

Loading

jagerber48 commented Nov 14, 2024

wshanks left a comment

wshanks Nov 14, 2024

jagerber48 Nov 14, 2024

wshanks Nov 14, 2024

jagerber48 Nov 14, 2024

wshanks Nov 14, 2024

jagerber48 Nov 14, 2024

jagerber48 Nov 14, 2024

jagerber48 commented Nov 14, 2024

jagerber48 commented Nov 14, 2024

jagerber48 commented Nov 14, 2024

jagerber48 commented Nov 15, 2024

jagerber48 commented Nov 15, 2024

jagerber48 commented Nov 15, 2024

newville left a comment

Make codecov pull requests informational only #269

Make codecov pull requests informational only #269

Conversation

jagerber48 commented Nov 13, 2024 • edited Loading

codecov bot commented Nov 13, 2024 • edited Loading

Codecov Report

jagerber48 commented Nov 14, 2024

wshanks left a comment

Choose a reason for hiding this comment

wshanks Nov 14, 2024

Choose a reason for hiding this comment

jagerber48 Nov 14, 2024

Choose a reason for hiding this comment

wshanks Nov 14, 2024

Choose a reason for hiding this comment

jagerber48 Nov 14, 2024

Choose a reason for hiding this comment

wshanks Nov 14, 2024

Choose a reason for hiding this comment

jagerber48 Nov 14, 2024

Choose a reason for hiding this comment

jagerber48 Nov 14, 2024

Choose a reason for hiding this comment

jagerber48 commented Nov 14, 2024

jagerber48 commented Nov 14, 2024

jagerber48 commented Nov 14, 2024

jagerber48 commented Nov 15, 2024

jagerber48 commented Nov 15, 2024

jagerber48 commented Nov 15, 2024

newville left a comment

Choose a reason for hiding this comment

jagerber48 commented Nov 13, 2024 •

edited

Loading

codecov bot commented Nov 13, 2024 •

edited

Loading