-
Notifications
You must be signed in to change notification settings - Fork 115
Enable cloning from jdbc to ixmp4 #610
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
Conversation
95f8d72 to
00d2867
Compare
|
I'll rebase this after the merge of #601. |
b7292ec to
e13bd9c
Compare
e13bd9c to
55ab270
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #610 +/- ##
======================================
Coverage 98.3% 98.3%
======================================
Files 51 51
Lines 6509 6624 +115
======================================
+ Hits 6399 6518 +119
+ Misses 110 106 -4
🚀 New features to boost your workflow:
|
55ab270 to
53460bb
Compare
|
Thanks for the solid start here @glatterf42, especially for the test setup to confirm that the operation works. I can join in/take over to make some adjustments. At a glance, I have these in mind:
I see see the new high-level method Scenario.iter_item_data() is called from within a lower-level/underlying class, JDBCBackend. The latter adjustment would avoid having this somewhat backward relationship. With the tests as a guardrail, I will try to make these changes as soon as I have a chance. |
- Document when .base.Backend.clone() should raise an exception of this type.
Backend-unaware clone of data associated with a TimeSeries object.
Ensure teardown code is executed even when fixture-using test functions raise exceptions.
53460bb to
21b7da7
Compare
21b7da7 to
04e3ed7
Compare
04e3ed7 to
2e4f112
Compare
- Separate setup to fixture functions; don't duplicate teardown code from _platform_fixture(). - Add and use assert_frame_equal_sorted(), to be tolerant of different data order in returned data frames. - Check the creation of a parameter that is not automatically present in the destination Scenario.
- Use a subclass of ixmp.core.item.Item instead of str. - Document which types a concrete Backend implementation must, or may not, support. - Adjust JDBCBackend, Scenario to match the updated signature.
- Backend-unaware clone of data associated with a Scenario object. - Scenario.clone() falls back to this function if Backend.clone() raises a CrossPlatformClone exception.
- Adjust signature to match .base.Backend() - Handle data for Equation and Variable by constructing ixmp4's preferred input data type directly. - Switch on item type at the top level, then loop over elements. - Narrow type of ._get_set_data(): - Sets may not contain float. - Sets may contain *only* int or str, not a mix of both.
- Avoid cross-import of upstream/underlying code and packages and ixmp4-specific code outside of .backend.ixmp4. - Raise CrossPlatformClone from JDBCBackend.clone() to trigger generic .core.scenario._clone().
2e4f112 to
ab623ec
Compare
khaeru
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @glatterf42 for developing this key feature and especially the tests for it, which made it easy to do some reorganization. This paves the way for similarly testing message_ix.Scenario.clone(): first to test databases, and eventually between centrally-operated IIASA Oracle and PostgreSQL databases.
At this point, it seems like ixmp per se is in good shape, and we are more waiting on:
- Review and merge of iiasa/ixmp4#208 by maintainers.
- Discussion on setting up and administering those central databases.
- PRs for message_ix and message-ix-models.
…none of which should be in scope for the next week. So I will merge this PR, and then we can revise if necessary as those other pieces catch up. In particular, if the upstream PR is still pending by the time the 3.12 milestone comes (now scheduled 23 January), I can put in appropriate safeguard or warnings for the next release
|
Thanks for finishing this and I agree with you assessment. Only one note to 1.: as I understand it, iiasa/ixmp4#219 will pick up the changes from iiasa/ixmp4#208, too, so only the former might be merged. Though I'm not entirely sure on this, so we might have to watch out for both PRs. |
In order to migrate from Oracle to PostgreSQL, we'll need a way to migrate (at least the most important) Scenarios. This PR adds a function that achieves just that, though there's still room for some improvement plus one open question.
@volker-krey In theory, you could use this branch to try cloning something from "ixmp_dev" to a local ixmp4/PostgreSQL DB (see iiasa/message_ix#981 on how to set one up).
More info:
main, and not v0.14.0 from PyPI.Important question
meta=True. Should I try to hack something together using ixmp_source directly or are we okay with saying one can only clone full Scenarios from JDBC to ixmp4 (in that regard; dropping the optimization solution is easy)?Possible improvements
bulk_insert()functions on the ixmp4 side: this is much more involved and requires implementing such functions in ixmp4 first, so if we don't mind performance so much, we could start an experiment with the current implementation.How to review
PR checklist