-
Notifications
You must be signed in to change notification settings - Fork 113
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 Backend, JDBCBackend, Model, GAMSModel classes #182
Conversation
- s_has_solution - s_list_items - s_item_index - s_item_elements
TimeSeries is clean of direct Java references
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.
@khaeru please see my 2 comments. I think we should have a meeting where you can present your vision, we can draw some diagrams to have common understanding of existing and planned architecture. I also commented on referenced issue #78 (comment)
ixmp/backend/base.py
Outdated
# Methods for message_ix.Scenario | ||
|
||
@abstractmethod | ||
def ms_cat_list(self, ms, name): |
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.
I didn't get the idea of having different methods for scenarios (s_
-prefixed) and message scenarios (ms_
-prefixed). It contradicts with original idea of ixmp package to be generic and potentially support many models.
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.
It contradicts with original idea of ixmp package to be generic and potentially support many models.
I agree. As I say in iiasa/message_ix#249, these are stack level violations where both ixmp and ixmp_source are currently not generic, but instead tailored to message_ix.
These were always there; this PR just makes it really obvious that they should not be there.
Our goal should be eventually to remove all ms_
methods.
- I already dealt with firstmodelyear and years_active by moving the logic into message_ix. The correspond ixmp_source methods are no longer used.
- The category/mapping set features should be moved back into ixmp.
- Backend (code like) ixmp_source should not treat ixmp.Scenario or its subclasses differently. Any special treatment of message_ix.Scenario should be in message_ix.Scenario.
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.
I see. Shouldn't then s_
prefixes be removed? As by default methods will operate on scenario (abstract API). Isn't it clear from parameter list and/or docstrings? Maybe also mark ms_
methods with deprecation comments as the code to be moved to message_ix
package
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.
Isn't it clear from parameter list
It should be, I guess, yes 🤔 Okay, I will make that change before merging, then.
mark
ms_
methods with deprecation comments
I think this code might end up in ixmp.Scenario, in which case it is just a name change rather than a deprecation and removal. But if this ends up going out as-is in the upcoming release, then I will add notices to explain that.
] | ||
|
||
|
||
class JDBCBackend(Backend): |
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.
I would not expose directly any references to JDBC essence. It should be hidden for consumer. E.g. in some future we may implement (java side) some additional storage options (nosql, file-based etc) which is not related to JDBC, but expose exactly same API (to python/R consumers).
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.
As mentioned in person, ixmp.Platform needs to choose and instantiate one Backend.
- For now, JDBCBackend is the only kind.
- Other Backends may provide storage without interacting with ixmp_source or any Java code.
- In the future, maybe, continued use of the "dbprops" and "dbtype" arguments could imply to use JDBCBackend, although I would prefer to clean up the semantics here.
Glad to do this. Who else besides us two should attend? |
Added a UML diagram to the PR description per discussions with @zikolach. |
Per a long discussion with @gidden and @zikolach today, I will merge this after @zikolach's approving review, and then open further issues for follow-up tasks including:
|
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.
Looks good for me!
I didn't do detailed python code review assuming most of the changes is just restructuring the code and API didn't break (as tests pass).
I would still consider keeping GAMS-related code on the Java side as to be used for:
- running integration tests (e.g. importing scenario (non-massage- specific on), running solver and writing it to DB)
- reusing code in web API backend application (e.g. export/import scenarios from scenario explorer)
- support for running GAMS-IXMP modeling from non-python environments (e.g. other JVM languages)
Thanks @khaeru for making changes, collecting feedback and keeping everyone informed!
ixmp/backend/base.py
Outdated
# Methods for message_ix.Scenario | ||
|
||
@abstractmethod | ||
def ms_cat_list(self, ms, name): |
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.
I see. Shouldn't then s_
prefixes be removed? As by default methods will operate on scenario (abstract API). Isn't it clear from parameter list and/or docstrings? Maybe also mark ms_
methods with deprecation comments as the code to be moved to message_ix
package
As described in this comment on #78, this PR creates a
Backend
class separating the Python ixmp.core classes from the ixmp_source Java classes; and aModel
class separating them from GAMS.There is a corresponding PR iiasa/message_ix#249.
How to review
Design
Why do this?
Several reasons…
Reading the code
to_gdx()
andread_sol_from_gdx()
will, for now, force Backend to know what GAMS is. IMO this is less than ideal; per Support non-GAMS models #119, in the future these could be moved to aclass GAMSModel(Model)
.PR checklist
ixmp.core._remove_ele
message_ix.core.Scenario
: ms_cat_set_elements, ms_cat_list, ms_cat_get_elements, ms_year_first_model, ms_years_active.Model
,GAMSModel
classes, and transfer: