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

Fix Code Quality CI: checkout action and complex function #320

Merged
merged 5 commits into from
Mar 28, 2025

Conversation

glatterf42
Copy link
Member

@glatterf42 glatterf42 commented Mar 26, 2025

This PR intends to fix a few things about the Code Quality CI:

  • It fixes a typo such that the Code Quality check will consider the changes here, not those already on main
  • It reduces the complexity of the cool_tech() function to below 11, which is our accepted limit here (@adrivinca, this may serve as a template for the future, if you're interested: I have simply extracted one if/else clause and moved that to its own function, following the idea that every function should have one clearly defined task)
  • I've also deleted the pre-commit cache in hopes of it now picking up the latest version of genno, which should fix the mypy errors we've been seeing in the nightly tests

Housekeeping:

How to review

  • Read the diff and note that the CI checks all pass.

PR checklist

  • Continuous integration checks all ✅
  • Add or expand tests; coverage checks both ✅
  • [ ] Add, expand, or update documentation. Just CI.
  • [ ] Update doc/whatsnew. Just CI.

@glatterf42 glatterf42 added the ci Continuous integration & testing label Mar 26, 2025
@glatterf42 glatterf42 self-assigned this Mar 26, 2025
@glatterf42
Copy link
Member Author

I have also deleted the cache entry called setup-uv-1-x86_64-unknown-linux-gnu-3.9-..., as this is used by the setup-uv action in the code quality run, but it doesn't seem to have any effect.
Also, even though the linked workflow file uses needs.check.outputs.ref for the code quality check now, the checkout action still resolves to a0d5420, the latest commit on main.

I'll have to do some organizing for the Community Meeting now and can only come back to this later, I'm afraid.

Copy link

codecov bot commented Mar 26, 2025

Codecov Report

Attention: Patch coverage is 90.00000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 77.4%. Comparing base (a0d5420) to head (2540694).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
message_ix_models/model/material/cli.py 0.0% 2 Missing ⚠️
message_ix_models/tools/wb.py 33.3% 2 Missing ⚠️
message_ix_models/report/operator.py 0.0% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main    #320     +/-   ##
=======================================
- Coverage   78.3%   77.4%   -0.9%     
=======================================
  Files        217     217             
  Lines      16913   16925     +12     
=======================================
- Hits       13254   13116    -138     
- Misses      3659    3809    +150     
Files with missing lines Coverage Δ
message_ix_models/model/bare.py 100.0% <100.0%> (ø)
message_ix_models/model/disutility.py 100.0% <100.0%> (ø)
message_ix_models/model/structure.py 100.0% <100.0%> (ø)
message_ix_models/model/transport/build.py 94.9% <100.0%> (ø)
...essage_ix_models/model/water/data/water_for_ppl.py 94.7% <100.0%> (+<0.1%) ⬆️
message_ix_models/project/edits/__init__.py 94.9% <100.0%> (+<0.1%) ⬆️
message_ix_models/project/ssp/structure.py 100.0% <100.0%> (ø)
message_ix_models/tests/model/test_disutility.py 100.0% <100.0%> (ø)
message_ix_models/tests/model/test_macro.py 100.0% <100.0%> (ø)
message_ix_models/tests/model/test_structure.py 100.0% <100.0%> (ø)
... and 7 more

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@khaeru
Copy link
Member

khaeru commented Mar 26, 2025

even though the linked workflow file uses needs.check.outputs.ref for the code quality check now, the checkout action still resolves to a0d5420, the latest commit on main.

I think that is expected. The web display on GitHub itself seems to be erroneous; the pull_request_target trigger always uses the version of the workflow file on the target branch, in this case main.

In order to debug, you can do what it says here:

# To debug the workflow, uncomment this entry AND comment pull_request_target
# pull_request:
# branches: [ main ]
pull_request_target:
branches: [ main, "migrate**" ]
types: [ labeled, opened, reopened, synchronize ]

in a commit with a message like "TEMPORARY Run 'pytest' workflow from PR branch"; preserve a link to the successful run; and then rebase the branch to drop the temporary commit.

@glatterf42
Copy link
Member Author

Thanks, I had forgotten that was necessary.

@adrivinca
Copy link
Contributor

Thanks, looks good to me. should we write a test for the new function?

@glatterf42
Copy link
Member Author

We could, but as long as cool_tech() is tested, the new function is already tested, too! That's the beauty of it: it's simply part of the larger function, so when cool_tech() runs, it also runs.
I'm trying to get the Code Quality check to pass, then we can worry about adding a test, but it's not strictly necessary here. Though it would be good practice.

@glatterf42 glatterf42 force-pushed the fix/code-quality-ci-03-25 branch 3 times, most recently from 78199ee to c20d20b Compare March 26, 2025 13:04
@glatterf42
Copy link
Member Author

Even adding the main branch of genno to the additional requirement for mypy does not seem to resolve the errors. I don't really understand what's going on there as all these type hints are very abstract. I could dive in, but I think I should be doing other things instead. So I would opt for applying type: ignore if we have tests for both places where mypy raises errors and these pass.

Unfortunately, there is now another error originating from the latest release of sdmx1, and I don't really have time to look into that, either :/

@khaeru
Copy link
Member

khaeru commented Mar 26, 2025

Unfortunately, there is now another error originating from the latest release of sdmx1, and I don't really have time to look into that, either :/

That's something I caused, so I'll step in with a commit to fix it.

@khaeru khaeru requested a review from macflo8 as a code owner March 27, 2025 07:31
@khaeru khaeru force-pushed the fix/code-quality-ci-03-25 branch from 55a3acb to b0a33e9 Compare March 27, 2025 07:38
@khaeru khaeru force-pushed the fix/code-quality-ci-03-25 branch 2 times, most recently from 8b06863 to 45aea12 Compare March 27, 2025 08:12
@khaeru
Copy link
Member

khaeru commented Mar 27, 2025

I'll step in with a commit to fix it.

I believe I've addressed the issues related to genno and sdmx1, so back to you @glatterf42.

Copy link
Contributor

@adrivinca adrivinca left a comment

Choose a reason for hiding this comment

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

At the end there were more changes elsewhere than in the water module, that looks fine for me. But have to admit I did not dig much in all the scripts.
You and Paul know what you are doing :)

khaeru added 2 commits March 28, 2025 09:18
Work around issue in sdmx1 v2.22.0: v30.Annotation cannot be written to
SDMX-ML.
- Use sdmx.model.{common,v21} as appropriate.
- Avoid warnings from sdmx1 v2.22 about import of Annotation from
  .common.
@glatterf42 glatterf42 force-pushed the fix/code-quality-ci-03-25 branch from 45aea12 to 2540694 Compare March 28, 2025 08:19
@glatterf42
Copy link
Member Author

These tests will fail again, at least in part, because they're using the code stored on main. Since they were passing before, I'll merge without waiting for them.

@glatterf42 glatterf42 merged commit 5ca9f9f into main Mar 28, 2025
22 of 23 checks passed
@glatterf42 glatterf42 deleted the fix/code-quality-ci-03-25 branch March 28, 2025 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous integration & testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants