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

Add ixmp4-backed platform functions #557

Draft
wants to merge 10 commits into
base: enh/ixmp4
Choose a base branch
from

Conversation

glatterf42
Copy link
Member

As suggested in #552 (comment), this PR adds IXMP4Backend functions that are required to pass the test_platform tests. This is separate to keep the PR reviews manageable.

As far as passing the test_platform tests go, this work is incomplete. In some places, I'm not sure best to proceed. For example, test_add_region and test_add_region_synonym are currently failing because the regions-DataFrame returned from the ixmp4 Backend does not include the data the tests currently expect. This is in turn due to the ixmp4 DB model for regions is different from the one in ixmp.
Most notably, ixmp4 requires the hierarchy parameter, but doesn't use parent or synonym. I asked @meksor and @danielhuppmann about this and they said that's on purpose and ixmp doesn't really use these parameters anymore, either. So I think it should be fine to adapt the existing tests, but please let me know what you think, @khaeru.

Similarly, ixmp4 does not seem to store timeslices in a similar way to the JDBC. It looks like I'll have to add this feature first, but I'm not really familiar with the IAMC data storage we currently have implemented.

Another missing feature is the ability to delete stuff in ixmp4. Both optimization data and items as well as Runs as a whole still lack this.

There are several more open questions marked as TODO inline. For example: ixmp4.Run.clone() doesn't support first_model_year and platform_dest. How crucial are these features?

How to review

TBD

PR checklist

  • Continuous integration checks all ✅
  • Add or expand tests; coverage checks both ✅
  • Add, expand, or update documentation.
  • [ ] Update release notes. Will be done in Implement ixmp4 shim #552.

@glatterf42 glatterf42 added enh New features & functionality python Pull requests that update Python code labels Jan 28, 2025
@glatterf42 glatterf42 self-assigned this Jan 28, 2025
@glatterf42 glatterf42 changed the base branch from main to enh/ixmp4 January 28, 2025 10:36
@khaeru
Copy link
Member

khaeru commented Jan 28, 2025

For example, test_add_region and test_add_region_synonym are currently failing because the regions-DataFrame returned from the ixmp4 Backend does not include the data the tests currently expect. This is in turn due to the ixmp4 DB model for regions is different from the one in ixmp.
Most notably, ixmp4 requires the hierarchy parameter, but doesn't use parent or synonym. I asked about this and they said that's on purpose and ixmp doesn't really use these parameters anymore, either. So I think it should be fine to adapt the existing tests, but please let me know what you think.

I think this is why we have iiasa/message_ix#894 and iiasa/message-ix-models#257. If this parent/synonym functionality is ‘load bearing’, then some test(s) in those PRs will fail. We also are able to search like this (within the IIASA org only) or more broadly to identify downstream repos that use the feature. These will help us answer the question “In order to avoid breaking changes,¹ does the shim/IXMP4Backend need extra code to emulate this behaviour of the JDBCBackend?” (We definitely do not want to put such changes into ixmp4.)

In some cases the answer to the question might turn out to be “No—we don't need this.” However, to reach there requires additional work (searching, reading code, discussion with actual users internal and external) that is not on the critical path to our goal: run the message_ix tutorials by May/June. There will also be many such cases. So I would say the way to proceed here is:

  1. Note in a code comment (NB Fails with ixmp4 because …) or with a pytest mark (@pytest.mark.xfail(..., reason="...") why the test is failing.
  2. Possibly link to a comment, issue or similar with more details. (Using issues might help us to catalogue those downstream usages and communicate with users about potential breaking changes.)
  3. Move on to other functionality / Backend API functions.

Once we finish with a 'first pass' over all the functionality, we can review all such cases and decide which need most attention. Things that would break more downstream code and be expensive to adapt to would get higher priority. Things where the answer turns out to be “No” as above we can communicate via migration instructions like “This is no longer possible with the shim/ixmp4 and will not be possible in the future,” and xfail the tests for IXMP4Backend only.

¹ To be precise, unacceptable breaking changes. Maybe in some cases we can find all of the downstream usage and the people responsible for maintaining it, and they will say, “Please go ahead, it is not a problem for me to update my code.”

@khaeru
Copy link
Member

khaeru commented Jan 28, 2025

With the above general strategy in mind:

Similarly, ixmp4 does not seem to store timeslices in a similar way to the JDBC. It looks like I'll have to add this feature first, but I'm not really familiar with the IAMC data storage we currently have implemented.

I would say this is non-critical/low priority:

  • Only one of the tutorials, westeros_seasonality.ipynb, uses sub-annual time resolution.
  • However, it does not interact with the IAMC-structured / TimeSeries data features.
  • We also don't typically include this tutorial in our introductory workshop.

Another missing feature is the ability to delete stuff in ixmp4. Both optimization data and items as well as Runs as a whole still lack this.

  • Again I'd look at the tutorials we use in the June workshop. I don't think we typically use features to delete sets or parameters. Thus, not critical/low priority.

There are several more open questions marked as TODO inline. For example: ixmp4.Run.clone() doesn't support first_model_year and platform_dest. How crucial are these features?

  • 2-platform clone is, I believe, widely used by our colleagues. It will also be needed for the strategy we discussed of splitting the huge ixmp-dev database schema into multiples, like ixmp-archive, ixmp-scratch, etc. (We discussed several times to trial this, but the additional schemas were not set up. Maybe it does not make sense to do this with Oracle and wait until we talk through ixmp4 to Postgres.) However, again, not used AFAIK in the workshop tutorials, thus lower priority.
  • first_model_year is an example of Disentangle message_ix and ixmp_source message_ix#254. Essentially:
    • An ixmp.Scenario/ixmp4.Run does not necessarily have a "year" set that has any particular meaning.
    • Thus it's a stack violation that their .clone() methods are aware of and handle a …year… parameter.
    • And therefore this is not something that IXMP4Backend or ixmp4 should or need to handle.
    • Test against ixmp4 message_ix#894 is a different matter. I'll add a comment there.

@glatterf42 glatterf42 force-pushed the enh/ixmp4-platform-functions branch from e898975 to e6d91f9 Compare February 4, 2025 10:10
@glatterf42 glatterf42 force-pushed the enh/ixmp4-platform-functions branch from e6d91f9 to 67c5639 Compare February 4, 2025 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enh New features & functionality python Pull requests that update Python code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants