Separate models.py into submodules

[khaeru]

This PR splits and moves the message_ix.models submodule to 4 new submodules:

• message_ix.common
• message_ix.macro
• message_ix.message
• message_ix.message_macro

This:

• Achieves better separation-of-concerns.

• Makes each submodule individually easier to understand and maintain. For instance, .message_macro is under 30 lines; .macro is under 100.

• Creates a more natural way to integrate further changes and improvements to the behaviour of one or another model. For example:

  • As I wrote at Test against ixmp4 #894 (review):

    Consolidate structural information from .util.scenario_data and behaviour from .util.scenario_setup into message_ix.model.message, since these are the structure/behaviour of a "MESSAGE-scheme Scenario" (as opposed to, say, a MACRO Scenario).

  • Add logic such as levelized_cost parameter is empty #971.
  • Add .models.shift_period() #873

  However, making those particular changes is out of scope for this PR.

There are no functional changes, except that importing some names from their old locations will give an DeprecationWarning.

How to review

TBA

PR checklist

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

[khaeru]

I am a little split on whether we could/should use a flatter structure here, for example:

• message_ix.common
• message_ix.macro —this would merge with an existing Python module; we could use clear submodule names like:
  • message_ix.macro.__init__
  • message_ix.macro.calibrate
• message_ix.message
• message_ix.message_macro

This would also avoid mixing Python and GAMS files in the existing subdirectory message_ix/model/, or having to rename those.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.2%. Comparing base (cc014c2) to head (df1bfa9).
⚠️ Report is 9 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #972     +/-   ##
=======================================
+ Coverage   96.1%   96.2%   +0.1%     
=======================================
  Files         54      60      +6     
  Lines       5097    5132     +35     
=======================================
+ Hits        4901    4940     +39     
+ Misses       196     192      -4     

Files with missing lines	Coverage Δ
message_ix/__init__.py	100.0% <100.0%> (ø)
message_ix/common.py	100.0% <100.0%> (ø)
message_ix/core.py	97.4% <100.0%> (ø)
message_ix/macro/__init__.py	100.0% <100.0%> (ø)
message_ix/macro/calibrate.py	96.7% <100.0%> (ø)
message_ix/message.py	100.0% <100.0%> (ø)
message_ix/message_macro.py	100.0% <100.0%> (ø)
message_ix/models.py	100.0% <100.0%> (+0.7%)	⬆️
message_ix/report/__init__.py	100.0% <100.0%> (ø)
message_ix/testing/__init__.py	99.7% <100.0%> (ø)
... and 7 more

[glatterf42]

I think ideally we'd like to have a structure that's intuitive to understand. When I see a file like message_ix/model/message.py, it's not immediately clear to me what to expect inside, especially if it's next to message_ix/model/MESSAGE_run.gms, message_ix/model/MESSAGE_project.gpr, .../MESSAGE_master.gms, and further in the MESSAGE/ subfolder. So I think I'd advocate for renaming files for clarity, either way, though this might be out of scope here as it would likely require changes (possibly also in the GAMS code).

The structure of the folders seems a little circular to me: we could split message_ix/ into different kinds of files first (e.g. a gams/ and python/ subdirectory), after which we'd split for message/ and macro/. This would keep all GAMS-related files in one place, but split up the MESSAGE-related files. Alternatively, if we split for message/ and macro/ first, we'd have all MESSAGE-related files in one place, but spread GAMS and Python files.

The latter seems a little more intuitive to me, but I'm not too sure. I think if we could set this up and want to invest the time, we could also make MACRO a submodule of message_ix (similar to how ruff is included in ty), which then follows the same structure as MESSAGE.

Sorry, I'm not sure there's much helpful in these thoughts.

[khaeru]

Okay, thanks for the inputs.

I guess if we also consider issues like #879, it can help distinguish:

1. Things (code, logic, data, structure) related to MESSAGE in general—i.e. the mathematical formulation of sets, parameters, and variables, abstract of how they are implemented.
2. Things related to the GAMS implementation of MESSAGE.
3. Things related to potential other/non-GAMS implementations of MESSAGE.
4. Each of (1), (2), (3) pertaining to each of MACRO and MESSAGE_MACRO.
5. Things related to Scenario objects—which may hold MESSAGE, MACRO, MESSAGE_MACRO, or other contents.

So, for instance, MESSAGE_MACRO as implemented in GAMS needs to include files from the GAMS implementations of each of MESSAGE and MACRO —this is one reason all these GAMS files are collected in the same directory. Currently that directory is named message_ix/model/. But if there were non-GAMS implementations of all 3, that directory name would be ambiguous.

I think I will (a) go with the flatter structure mentioned in my previous comment and (b) leave the directory name …/model/ for now. In the future (another PR, maybe in this release or the next) we can maybe rename that to …/gams/, split it up, or something else.

[glatterf42]

Sounds good, thanks :)

[glatterf42]

Thanks, looks good to me aside from some documentation references that may have been invalidated here.

[glatterf42]

I'm looking at the docs build and I'm surprised to see lots and lots of errors. Most of them already existed for the latest build, though, it seems. What's new is that doc/macro.rst complains about not being able to import various things from message_ix.macro. I'm guessing they moved somewhere (too many to list, but starting here in the build logs).

I also notice that there is an actual error about the table that contains PRICE_COMMODITY in model_core.rst, which is likely just imported from the GAMS file. This seems to be malformed, so is not rendered in our docs. While this error isn't new, it would be nice to fix it to have our docs be complete again. The error says "Bottom/header table border does not match top border.", which sounds like we just have a mismatch in the number of = used for the borders.

[glatterf42]

doc/install-adv.rst and doc/reporting.rst also complain about missing reference targets, as well as the RELEASE_NOTES.rst, curiously.

[khaeru]

Thanks, I'll fix up these examples you mention. It would be good to have someone do a more systematic sweep, at some point.

[khaeru]

the table that contains PRICE_COMMODITY in model_core.rst,

This happened in #451: https://github.com/iiasa/message_ix/blame/cc014c24c7084f252f3827a86187a0ad467155a5/message_ix/model/MESSAGE/model_core.gms#L150

I've changed it to a ReST list-table, which makes it easier to edit.

doc/macro.rst complains about not being able to import various things from message_ix.macro

That file is now message_ix.macro.calibrate. Adjusted.

doc/install-adv.rst

home/khaeru/vc/iiasa/message-ix/doc/install-adv.rst:71: WARNING: py:mod reference target not found: jpype [ref.mod]

Like a lot of Sphinx packages, the intersphinx inventory for JPype1 does not actually contain a target for the top-level module:

$ python -m sphinx.ext.intersphinx https://jpype.readthedocs.io/en/stable/objects.inv | grep -B1 -A6 ^py:module
    jpype.dbapi2.JDBCType.set         : dbapi2.html#jpype.dbapi2.JDBCType.set
py:module
    jpype.beans                       : api.html#module-jpype.beans
    jpype.imports                     : api.html#module-jpype.imports
    jpype.pickle                      : api.html#module-jpype.pickle
    jpype.types                       : api.html#module-jpype.types
py:property
    jpype.dbapi2.Connection.adapters  : dbapi2.html#jpype.dbapi2.Connection.adapters

I added it to reference_aliases in doc/conf.py alongside the other such cases.

/home/khaeru/vc/iiasa/message-ix/doc/install-adv.rst:233: WARNING: py:class reference target not found: IXMP4Backend [ref.class]

This one looked like:

message_ix/doc/install-adv.rst

Line 233 in cc014c2

- ``ixmp4`` includes packages require to use :class:`ixmp.IXMP4Backend <.IXMP4Backend>`,

The shorthand whereby .IXMP4Backend will find a fully-qualified name (FQN) like ixmp.backend.ixmp4.IXMP4Backend only functions within the same project, not across projects via intersphinx. I replaced with the FQN.

doc/reporting.rst

Most of these cases occur with references that work/are valid within the genno docs, but are not properly resolved when those docs are rendered within the context of the message_ix docs. Some of those might be resolvable by copying some of the genno doc/conf.py settings. I'll omit those for now.

RELEASE_NOTES.rst

/home/khaeru/vc/iiasa/message-ix/RELEASE_NOTES.rst:43: WARNING: py:mod reference target not found: message_ix.common [ref.mod]
/home/khaeru/vc/iiasa/message-ix/RELEASE_NOTES.rst:44: WARNING: py:mod reference target not found: message_ix.macro [ref.mod]
/home/khaeru/vc/iiasa/message-ix/RELEASE_NOTES.rst:45: WARNING: py:mod reference target not found: message_ix.message [ref.mod]
/home/khaeru/vc/iiasa/message-ix/RELEASE_NOTES.rst:46: WARNING: py:mod reference target not found: message_ix.message_macro [ref.mod]

Added the appropriate targets.

/home/khaeru/vc/iiasa/message-ix/RELEASE_NOTES.rst:89: WARNING: py:class reference target not found: IXMP4Backend [ref.class]
/home/khaeru/vc/iiasa/message-ix/RELEASE_NOTES.rst:302: WARNING: py:mod reference target not found: macro [ref.mod]

Same as above.

/home/khaeru/vc/iiasa/message-ix/RELEASE_NOTES.rst:198: WARNING: py:meth reference target not found: message_ix.models.GAMSModel [ref.meth]

This was trying to use :meth: to reference a :class:. Fixed.

/home/khaeru/vc/iiasa/message-ix/RELEASE_NOTES.rst:449: WARNING: py:mod reference target not found: message_ix.testing.nightly [ref.mod]

This happens when things are removed. I resolve these by using the :py: role, which looks code-like but no longer tries to link to anything.

[glatterf42]

Thanks, this already looks a lot better. And agreed that a more systematic sweep would be useful, but out of scope here :)