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

Implement ixmp4 shim #552

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from
Draft

Implement ixmp4 shim #552

wants to merge 15 commits into from

Conversation

khaeru
Copy link
Member

@khaeru khaeru commented Nov 28, 2024

This PR is intended to aid @glatterf42 in developing model data features in ixmp4. It is not a complete plan or solution for migrating ixmp-based code, workflows, and research to ixmp4; such a plan would involve many additional elements that are not in scope for this PR.

The PR:

  • Adds a IXMP4Backend class that allows ixmp.Platform, ixmp.TimeSeries, and ixmp.Scenario (thus all existing code based on ixmp and message_ix) to talk to ixmp4's API. This functions as a shim between the established and new/developing APIs.
  • Makes minimal changes to allow ixmp.Platform backed by IXMP4Backend to be created and used.
  • Adjusts or parametrizes tests of the ixmp Platform, TimeSeries, and Scenario classes to run against both JDBCBackend and IXMP4Backend, ensuring the latter provides the same behaviour expected of the former per the test suite.
    • This is a precondition for migrating other code (for instance, in message-ix-models or message_data) to, first, use ixmp4 via an IXMP4Backend, and eventually, ixmp4 directly.
  • Serves as a basis for Test against ixmp4 message_ix#894 and Test against ixmp4 message-ix-models#257.

Notes

  • This PR recreates Implement ixmp4 shim #516 with a new branch name. Using the same branch name in ixmp and message_ix allows to minimize the changes to CI in message-ix-models.
  • ixmp4 is not published on PyPI for Python 3.9 and can't be installed in those CI jobs. Possible ways to respond:
    • Separate ixmp4 from the tests dependency, and don't install it on those Pythons.
    • Adjust parametrized tests to not import ixmp4 or run against IXMP4Backend with those Pythons.
    • Document that IXMP4Backend is only available for Python >= 3.10.

How to review

TBD. This branch/PR may not necessarily be merged itself, but may serve as the basis for another PR that adds the IXMP4Backend.

PR checklist

  • Continuous integration checks all ✅
  • Add or expand tests; coverage checks both ✅
  • Add, expand, or update documentation.
  • Update release notes.

@khaeru khaeru added the enh New features & functionality label Nov 28, 2024
khaeru added a commit to iiasa/message_ix that referenced this pull request Nov 28, 2024
khaeru added a commit to iiasa/message-ix-models that referenced this pull request Nov 28, 2024
khaeru added a commit to iiasa/message-ix-models that referenced this pull request Dec 3, 2024
khaeru added a commit to iiasa/message-ix-models that referenced this pull request Jan 9, 2025
@glatterf42
Copy link
Member

Thanks for starting this PR! I have reworked the setup part of the tests so that test_mp, mp, and test_mp_f are now parametrized for "jdbc" and "ixmp4". Please take a look and tell me what you think :)
In particular, I think some parts are not as robust as they could be, which mainly comes down to me not knowing how we want them to look. For example, IXMP4BACKEND.__init__ takes only **kwargs for now, though I would like it to take something like name: str, dsn: str | None = None, and require dsn to be there if the platform name is not known to ixmp4.

Aside from questions like this, I think the tests are looking good: most [ixmp4] tests are failing with NotImplementedErrors, but by adding the missing functions, we should see those disappear gradually. I think I should use separate branches for that and keep this branch for the main test setup.

@glatterf42
Copy link
Member

I'm not sure if the sqlite dsn ":sqlite:///:memory:" is sufficient for running multiple tasks on the same machine. We might have to integrate the unique platform_name here, too, but at least sqlite's docs say that each DB located at ":memory:" is distinct from all others, so it might also work just fine.

@khaeru
Copy link
Member Author

khaeru commented Jan 20, 2025

[H]ow we want them to look. For example, IXMP4BACKEND.__init__ takes only **kwargs for now, though I would like it to take something like name: str, dsn: str | None = None, and require dsn to be there if the platform name is not known to ixmp4.

Have a look at JDBCBackend.handle_config():

ixmp/ixmp/backend/jdbc.py

Lines 341 to 397 in 58e801c

# Platform methods
@classmethod
def handle_config(cls, args, kwargs):
"""Handle platform/backend config arguments.
`args` will overwrite any `kwargs`, and may be one of:
- ("oracle", url, user, password, [jvmargs]) for an Oracle database.
- ("hsqldb", path, [jvmargs]) for a file-backed HyperSQL database.
- ("hsqldb",) with "url" supplied via `kwargs`, e.g. "jdbc:hsqldb:mem://foo" for
an in-memory database.
"""
args = list(args)
info = copy(kwargs)
# First argument: driver
try:
info["driver"] = args.pop(0)
except IndexError:
raise ValueError(
f"≥1 positional argument required for class=jdbc: driver; got: {args}, "
+ str(kwargs)
)
if info["driver"] == "oracle":
if len(args) < 3:
raise ValueError(
"3 or 4 arguments expected for driver=oracle: url, user, password, "
f"[jvmargs]; got: {str(args)}"
)
info["url"], info["user"], info["password"], *jvmargs = args
elif info["driver"] == "hsqldb":
try:
info["path"] = Path(args.pop(0)).resolve()
except IndexError:
if "url" not in info:
raise ValueError(
"must supply either positional path or url= keyword argument "
"for driver=hsqldb"
)
jvmargs = args
else:
raise ValueError(
f"driver={info['driver']}; expected one of {set(DRIVER_CLASS)}"
)
if len(jvmargs) > 1:
raise ValueError(
f"Unrecognized extra argument(s) for driver={info['driver']}: "
f"{jvmargs[1:]}"
)
elif len(jvmargs):
info["jvmargs"] = jvmargs[0]
return info

As it stands, this method is called via Config.add_platform(); either from code or using args from the CLI. The dict returned by handle_config() is then stored in the config (file), and should contain only valid keyword arguments for the Backend.__init__ method of the matching Backend subclass. For instance, JDBCBackend.handle_config() returns a dict that should be valid kwargs for JDBCBackend.__init__. So IXMP4Backend.handle_config() could ensure there is a DSN, or supply a default, e.g. based on whether the user selects SQLite or Postgres, or has the required package(s) installed.

This mechanism is pretty old, e.g. it predates mature Python dataclasses, which would probably be a cleaner way to write something like this (if we were starting from scratch today).

@glatterf42
Copy link
Member

Thanks for the suggestion :)
I'm not sure how handle_config() can help me in this case, though. I'd like to make the logic handling of e.g. dsn more robust, but this is (so far) only needed in one case. Most of the tests in test_platform use the mp or test_mp fixtures, which always call ixmp_config.add_platform() for either kind of backend. Only test_init1 doesn't use it and instead checks that a platform can still be instantiated. So it would feel like cheating to add_platform manually in that test if backend == "ixmp4". And since the test just calls platform.__init__(), it will only read the config, not add new things.

Maybe I'll get a better feeling for what we need or what's helpful by first trying to add some other functions that should already be supported by ixmp4 :)

@khaeru
Copy link
Member Author

khaeru commented Jan 21, 2025

Maybe I'll get a better feeling for what we need or what's helpful by first trying to add some other functions that should already be supported by ixmp4 :)

Yes, agreed—I'd say our first target is to get some or more tests passing, and then we can look at tidying up any hacks that are required to get to that point. As long as we mark them clearly (# FIXME or in commit messages) we can come back and find those.

@glatterf42 glatterf42 force-pushed the enh/ixmp4 branch 2 times, most recently from 81de173 to 2bf2493 Compare January 21, 2025 13:56
Copy link

codecov bot commented Jan 21, 2025

Codecov Report

Attention: Patch coverage is 71.96970% with 37 lines in your changes missing coverage. Please review.

Project coverage is 98.1%. Comparing base (58e801c) to head (3bc8760).

Files with missing lines Patch % Lines
ixmp/backend/ixmp4.py 67.0% 28 Missing ⚠️
ixmp/testing/__init__.py 74.2% 9 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main    #552     +/-   ##
=======================================
- Coverage   98.8%   98.1%   -0.8%     
=======================================
  Files         44      45      +1     
  Lines       4790    4898    +108     
=======================================
+ Hits        4737    4807     +70     
- Misses        53      91     +38     
Files with missing lines Coverage Δ
ixmp/__init__.py 100.0% <100.0%> (ø)
ixmp/_config.py 96.1% <ø> (-0.7%) ⬇️
ixmp/testing/data.py 97.3% <100.0%> (ø)
ixmp/tests/core/test_platform.py 100.0% <100.0%> (ø)
ixmp/testing/__init__.py 90.0% <74.2%> (-6.0%) ⬇️
ixmp/backend/ixmp4.py 67.0% <67.0%> (ø)

... and 3 files with indirect coverage changes

glatterf42 pushed a commit to iiasa/message_ix that referenced this pull request Jan 28, 2025
khaeru added a commit to iiasa/message-ix-models that referenced this pull request Jan 31, 2025
glatterf42 pushed a commit to iiasa/message_ix that referenced this pull request Feb 4, 2025
khaeru added a commit to iiasa/message-ix-models that referenced this pull request Feb 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enh New features & functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants