Skip to content

Conversation

@glatterf42
Copy link
Member

@glatterf42 glatterf42 commented Sep 8, 2025

Closes #975, supersedes #976. The former PR was an attempt to pin the ixmp4 version, which doesn't work as explained there. Instead, as @khaeru suggested, we should bump the minimum supported version of ixmp4 in ixmp to v0.12. This PR adjusts the code we have in message_ix that runs into RunLockRequired errors by calling ixmp4-backend functions directly.

These functions were exclusively necessary in scenario_setup.py, which is code that is called for every Scenario, anyway, and presumably not by user code, but by internal routines, so it should be safe to use backend functions directly.

Please note that this is not an attempt to integrate the Run.transact() or other ixmp4 rollback features.

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 internal function calls.
  • [ ] Update release notes. Just internal function calls.

@glatterf42 glatterf42 added this to the 3.12 milestone Sep 8, 2025
@glatterf42 glatterf42 self-assigned this Sep 8, 2025
@glatterf42 glatterf42 added the bug Doesn't work as advertised/unintended effects label Sep 8, 2025
@codecov
Copy link

codecov bot commented Sep 8, 2025

Codecov Report

❌ Patch coverage is 84.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.2%. Comparing base (8f55fcf) to head (41ae7db).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
message_ix/util/scenario_setup.py 84.0% 4 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main    #977   +/-   ##
=====================================
  Coverage   96.2%   96.2%           
=====================================
  Files         60      60           
  Lines       5132    5135    +3     
=====================================
+ Hits        4940    4943    +3     
  Misses       192     192           
Files with missing lines Coverage Δ
message_ix/util/ixmp4.py 100.0% <ø> (ø)
message_ix/util/scenario_setup.py 95.6% <84.0%> (+0.1%) ⬆️

@glatterf42 glatterf42 requested a review from khaeru September 8, 2025 12:02
Copy link
Member

@khaeru khaeru left a comment

Choose a reason for hiding this comment

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

we should bump the minimum supported version of ixmp4 in ixmp to v0.12.

As I understand, there will be a separate PR (not yet created) in ixmp to make this change. For the moment, I look at the CI jobs and see that ixmp4 v0.12.0 is being installed by virtue of the installer picking the most recent release.

Approved because those "pytest" jobs pass. I'm not sure whether the intent here is also to ensure test coverage of missing lines—it's OK if not, we should just plan to do that when the next opportunity arises.

@glatterf42
Copy link
Member Author

Thanks, I'll open such a PR in ixmp today or tomorrow :)
Increasing the test coverage was not on my goals for this PR, and all missing lines are lines that were also missing before. So I'll merge this PR, but agree that it would be good to increase coverage further.
Though maybe that's also unnecessary: after all, the functions in scenario_setup set up the default data (and ensure that e.g. the values in the year indexset are ordered). It seems there are no cases where more than one element is added to an indexset during this default setup (the list-branch is not called) and that storage in ixmp4 keeps the year indexset ordered, so this branch is not triggered either. Thus, these are mainly safeguards at this point and we don't need them to run Scenarios :)

@glatterf42 glatterf42 merged commit 33edac8 into main Sep 8, 2025
46 of 52 checks passed
@glatterf42 glatterf42 deleted the fix/adjust-to-ixmp4-v0.12.0 branch September 8, 2025 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Doesn't work as advertised/unintended effects

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Adjust to ixmp4 dependency bump

2 participants