Collect water module settings into Config dataclass#509
Conversation
Wegatriespython
added
the
safe to test
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #509 +/- ##
=====================================
Coverage 74.1% 74.2%
=====================================
Files 318 320 +2
Lines 25566 25655 +89
=====================================
+ Hits 18962 19047 +85
- Misses 6604 6608 +4
🚀 New features to boost your workflow:
|
khaeru
left a comment
khaeru
left a comment
There was a problem hiding this comment.
So far this looks great! I guess IDE tools can now jump from cfg.x references in the various functions to the places where Config.x are defined.
I see the RTD build is failing, but that is because of bugs upstream in ixmp4/pyam-iamc that I will try separately to mitigate. Those mitigations might belong here, or in message_ix or ixmp; whatever the case I'll notify to either re-run or rebase once they are in place.
Also adds a fix for nested CLI option handling so an omitted subcommand --ssp does not clear a value already set on the parent water-ix command.
I see how this fix works, but I am struggling a little bit to think about use cases. It points to possibilities like this:
mix-models foo bar … # (1)
mix-models foo --ssp=SSP1 bar … # (2)
mix-models foo bar --ssp=SSP1 … # (3)
mix-models foo --ssp=SSP1 bar --ssp=SSP1 … # (4)
mix-models foo --ssp=SSP1 bar --ssp=SSP2 … # (5)
What would be a case of 'foo' and 'bar' such that (2) and (3) need to be distinguished? Where (4) should change behaviour vs (2) and (3)? Where (5) must be supported?
| assert "ZMB" == result.output.strip() | ||
|
|
||
|
|
||
| def test_scenario_param_preserves_outer_value(mix_models_cli): |
There was a problem hiding this comment.
Adding type hints to the signature (even -> None: at the end of line) would allow mypy to check the contents.
| results = {} | ||
|
|
||
| # Reference to the water configuration | ||
| cfg = Config.from_context(context) |
There was a problem hiding this comment.
One thing that's worth thinking about briefly: is it the duty of these individual functions, or of higher-level code, to ensure the Context instance is properly prepared with a Config instance?
If the latter, this code could simply do:
cfg: "Config" = context.water…which would raise an exception if parent/calling code had not set up the proper context.
I believe it should work fine either way, since the way .water.Config.from_context() is currently written ensures that 2nd, 3rd, etc. calls will always get the same Config instance as the first (thus, the same settings). (It does differ slightly from .transport.Config.from_context(), which always creates a new instance. But IMO this difference is fine, for now, as long as the method docstrings are clear.)
There was a problem hiding this comment.
I think making it stricter makes sense, but currently there are a fair bit of changes coming up on water and I would like to leave it more defensive, with a docstring update.
| """Settings for :mod:`message_ix_models.model.water`.""" | ||
|
|
||
| #: Water build variant. | ||
| nexus_set: Literal["cooling", "nexus"] = "nexus" |
There was a problem hiding this comment.
This is great for now. In some cases it might make sense, in the future, to add an Enum, which can also help to sanitize user input:
class NexusSet(Enum):
#: (can add verbose description here)
cooling = auto()
#: (ditto)
nexus = auto()
class Config(ConfigHelper):
nexus_set: NexusSet = NexusSet.nexus
def __post_init__(self) -> None:
if isinstance(self.nexus_set, str):
self.nexus_set = NexusSet[self.nexus_set]
|
So the issue is more the water cli takes in some redundancies for options, which would be good to clean up as well. Currently for the water-ix group, With the fix, 2 and 3 will produce SSP1 and 5 will deliver SSP2.
|
OK, agreed that cleaning up those redundancies (in a future PR) would be the preferred approach, but again the current fix is good for the code as it is. In general/the long run, my preference would be:
|
adrivinca
left a comment
adrivinca
left a comment
There was a problem hiding this comment.
Also had a look. great tidy up!
I think it can be merged if the test failures are solved (and not due to changes here)
Now hopefully done in #511. Please rebase on |
Add message_ix_models.model.water.config.Config holding water-module settings/state (RCP, SDG, REL, time, type_reg, map_ISO_c, nexus_set, reduced-basin settings, valid_basins, all_nodes). Build/data/utility code reads Config.from_context(context); the CLI populates context.water at setup. Config.apply() remains as an explicit backward-compat helper for callers that build a Context directly.
store_context() now skips assignment when value is None and the parameter is already set on the Context. Keeps a group-level `water-ix --ssp SSPx` alive when a subcommand declares its own --ssp with default=None; the subcommand callback used to overwrite the group's value at the end of parsing.
Wegatriespython
force-pushed
the
wr/pr-c-water-config
branch
from
89df9d4 to
6110b41
Compare
3 participants
Introduce
message_ix_models.model.water.Configand move water-module state from loose top-levelContextattributes tocontext.water.The CLI now initializes and updates this config object for water options such as
RCP,SDG,REL, time slices, region type, and reduced-basin settings. Build, data, and utility code read the same settings throughConfig.from_context(context), matching the configuration pattern already used by transport and reporting. Following recommendation at the end of #432 from @khaeruAlso adds a fix for nested CLI option handling so an omitted subcommand
--sspdoes not clear a value already set on the parentwater-ixcommand.How to review
Look at the diff, check if the change to
click.pymight have un-intended side effects for other workflows.PR checklist