iiasa / message-ix-models

Tools for the MESSAGEix-GLOBIOM family of models
https://docs.messageix.org/models
Apache License 2.0
17 stars 33 forks source link

Save minutes by avoiding duplicate test runs #150

Closed glatterf42 closed 6 months ago

glatterf42 commented 8 months ago

This PR removes the trigger for CI runs on pushed to main to avoid duplicate test runs. This avoids duplicate runs.

In order for this to work properly, we should make sure that

We might also want to

All of which can be done via the settings -> branches.

For now, we require the py3.11 checks for macos, ubuntu, and windows and message_ix/main to be successful. We should include checks for message_ix v3.8.0 (or only use main) and could try using py3.12, too.

How to review

PR checklist

codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 75.4%. Comparing base (583d3e9) to head (8535525).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #150 +/- ## ===================================== Coverage 75.4% 75.4% ===================================== Files 93 93 Lines 6211 6214 +3 ===================================== + Hits 4684 4687 +3 Misses 1527 1527 ``` | [Files](https://app.codecov.io/gh/iiasa/message-ix-models/pull/150?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iiasa) | Coverage Δ | | |---|---|---| | [message\_ix\_models/tests/test\_util.py](https://app.codecov.io/gh/iiasa/message-ix-models/pull/150?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iiasa#diff-bWVzc2FnZV9peF9tb2RlbHMvdGVzdHMvdGVzdF91dGlsLnB5) | `100.0% <100.0%> (ø)` | | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/iiasa/message-ix-models/pull/150/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iiasa)
khaeru commented 8 months ago

We should include checks for message_ix v3.8.0 (or only use main)

Yes, including both the latest release and main should be our general rule.

and could try using py3.12, too.

Worth a try! I not yet sure if all of the direct dependencies here are compatible with Python 3.12, but we could find out.

glatterf42 commented 8 months ago

Let's see how the 3.12 tests are doing. We might have to re-trigger a check evaluation to get rid of the '3.11-required' checks and update the branch protection rule if we want to keep the 3.12 version.

khaeru commented 8 months ago

We might have to re-trigger a check evaluation to get rid of the '3.11-required' checks and update the branch protection rule

I find if I modify the required checks, save settings, and then refresh the PR page, then the new requirements are shown without the need to re-run any jobs.

However it looks like there are several failures, so one option would be to park some notes in an issue to be addressed separately and limit this PR to adding ixmp/message_ix 3.8.0. Or try to also address them in this PR—up to you.

glatterf42 commented 8 months ago

This PR now refines the pytest workflow usage of python-version. A lot of the failures originated from the incompatibility of (ixmp, message_ix) < 3.8.0 with python 3.12 and we are not requiring this compatibility anywhere, so this workflow shouldn't require it either.

glatterf42 commented 8 months ago

@khaeru I'm not sure how to handle the Code Quality issue (which is caused by pint missing library stubs or py.typed markers). Locally, this is the same for me and since pint doesn't support 3.12 officially yet, there might not be much we can do aside from adding it to tool.mypy.overrides in pyproject.toml until official support is added. What confuses me, though, is that recent runs of code quality checks on ixmp seem to have installed python 3.12 and successfully run mypy, even though pint is a dependency there, too, and not excluded via pyproject.toml.

khaeru commented 8 months ago
glatterf42 commented 8 months ago

Thanks for the explanation. If I didn't misunderstand it entirely, f7d54e3 should be exactly the change you suggested. However, this doesn't solve the problem here (and probably won't for message_data, either, since the error message (also for message_data)) reads:

message_data/testing.py:7: error: Skipping analyzing "pint": module is installed, but missing library stubs or py.typed marker  [import-untyped]
glatterf42 commented 8 months ago

As far as I can tell, there's now no difference between the failing Code quality run here and e.g. this successful one for ixmp except for the packages being passed to pre-commit's mypy-venv.

[INFO] Initializing environment for local:mypy >= 1.8.0,plotnine,pytest,sdmx1,types-PyYAML,types-tqdm,ixmp @ git+https://github.com/iiasa/ixmp.git@main,message-ix @ git+https://github.com/iiasa/message_ix.git@main.

here vs

[INFO] Initializing environment for local:mypy >= 1.8.0,genno,GitPython,nbclient,pytest,sphinx,xarray.

there. Since it doesn't seem to be missing pint, my guess is that one of the dependencies here (most likely message-ix) depends on pint in a way that just ixmp doesn't require to work.

P.S.: there's also an environment variable called depth set to 100 for ixmp, but that doesn't seem to affect this workflow.

glatterf42 commented 7 months ago

Okay, so for the last CI check before the force-push, we had: https://github.com/iiasa/message-ix-models/blob/cba07b2e7864ad16bb20367ff7ece11a4629776c/.github/workflows/pytest.yaml#L111 commented out and https://github.com/iiasa/message-ix-models/blob/cba07b2e7864ad16bb20367ff7ece11a4629776c/.pre-commit-config.yaml#L11-L12 appended with ; python -m pip list at the end of the quotation marks. For some reason, this made the code quality check successful, but it's failing again now.

glatterf42 commented 7 months ago

For some reason, the Codecov action v4 keeps failing in this repository, while it works fine with ixmp and message_ix and I don't remember setting anything up differently between them.

glatterf42 commented 7 months ago

For some reason, the Codecov action v4 keeps failing in this repository, while it works fine with ixmp and message_ix and I don't remember setting anything up differently between them.

At least this seems clear now: I forgot to pass the CODECOV_TOKEN secret to the action. On the other repos, the PR is coming from a fork, which triggers a tokenless upload.

glatterf42 commented 7 months ago

There it is again: adding ; python -m pip list to the pre-commit-config bash mypy call makes the code quality CI check successful. Let's see if the same happens for message_data.

glatterf42 commented 7 months ago

@khaeru Gotta be honest: I don't understand what's going on here, but the tests are successful here and over at https://github.com/iiasa/message_data/pull/533, so it seems like we can adapt the required-successful checks to check python 3.12 and officially support 3.12 throughout the message stack :)

glatterf42 commented 6 months ago

The publish workflow doesn't run because https://github.com/iiasa/message-ix-models/actions/runs/8433520273 is neither starting (despite the job it's waiting for being finished) nor able to get cancelled. If it's okay with you, @khaeru, I'd change the required checks and merge this, anyway.

khaeru commented 6 months ago

Could you please adjust the required statuses in the repo settings? These have different names now with these changes to the workflow. Otherwise good to go.