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

feat: ngen_cal_model_configure model hook #161

Merged
merged 11 commits into from
Aug 15, 2024
19 changes: 16 additions & 3 deletions python/ngen_cal/src/ngen/cal/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
import yaml
from os import chdir
from pathlib import Path
from ngen.cal.configuration import General
from ngen.cal.configuration import General, Model
from ngen.cal.ngen import Ngen
from ngen.cal.search import dds, dds_set, pso_search
from ngen.cal.strategy import Algorithm
from ngen.cal.agent import Agent
Expand Down Expand Up @@ -42,6 +43,13 @@ def main(general: General, model_conf: Mapping[str, Any]):
import numpy as np
np.random.seed(general.random_seed)

# model scope plugins setup in constructor
model = Model(model=model_conf)

# NOTE: if support for new models is added, this will need to be modified
assert isinstance(model.model, Ngen), f"ngen.cal.ngen.Ngen expected, got {type(model.model)}"
model_inner = model.model.unwrap()

plugins = cast(List[Union[Callable, ModuleType]], general.plugins)
plugin_manager = setup_plugin_manager(plugins)

Expand All @@ -57,8 +65,13 @@ def main(general: General, model_conf: Mapping[str, Any]):
into a single variable vector and calibrating a set of heterogenous formultions...
"""
start_iteration = 0
#Initialize the starting agent
agent = Agent(model_conf, general.workdir, general.log, general.restart, general.strategy.parameters)

# Initialize the starting agent
agent = Agent(model, general.workdir, general.log, general.restart, general.strategy.parameters)
Comment on lines +68 to +70
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Initialize the starting agent
agent = Agent(model, general.workdir, general.log, general.restart, general.strategy.parameters)


# Agent mutates the model config, so `ngen_cal_model_configure` is called afterwards
model_inner._plugin_manager.hook.ngen_cal_model_configure(config=model_inner)

if general.strategy.algorithm == Algorithm.dds:
func = dds_set #FIXME what about explicit/dds
start_iteration = general.start_iteration
Expand Down
14 changes: 14 additions & 0 deletions python/ngen_cal/src/ngen/cal/_hookspec.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from hypy.nexus import Nexus

from ngen.cal.configuration import General
from ngen.cal.model import ModelExec
from ngen.cal.meta import JobMeta

hookspec = pluggy.HookspecMarker(PROJECT_SLUG)
Expand Down Expand Up @@ -51,6 +52,19 @@ def ngen_cal_finish(exception: Exception | None) -> None:


class ModelHooks:
@hookspec
def ngen_cal_model_configure(self, config: ModelExec) -> None:
"""
Called before calibration begins.
This allow plugins to perform initial configuration.

Plugins' configuration data should be provided using the
`plugins_settings` field in the `model` section of an `ngen.cal`
configuration file.
By convention, the name of the plugin should be used as top level key in
the `plugin_settings` dictionary.
"""

@hookspec(firstresult=True)
def ngen_cal_model_observations(
self,
Expand Down
21 changes: 12 additions & 9 deletions python/ngen_cal/src/ngen/cal/agent.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

from abc import ABC, abstractmethod
from ngen.cal.meta import JobMeta
from ngen.cal.configuration import Model
from ngen.cal.configuration import Model, NoModel
from ngen.cal.utils import pushd
from pathlib import Path
from typing import TYPE_CHECKING
Expand Down Expand Up @@ -59,9 +59,14 @@ def best_params(self) -> str:

class Agent(BaseAgent):

def __init__(self, model_conf, workdir: Path, log: bool=False, restart: bool=False, parameters: Mapping[str, Any] | None = {}):
def __init__(self, model: Model, workdir: Path, log: bool=False, restart: bool=False, parameters: Mapping[str, Any] | None = {}):
self._workdir = workdir
self._job = None
assert not isinstance(model.model, NoModel), "invariant"
# NOTE: if support for new models is added, support for other model
# type variants will be required
ngen_model = model.model.unwrap()
self._model = model
if restart:
# find prior ngen workdirs
# FIXME if a user starts with an independent calibration strategy
Expand All @@ -72,17 +77,15 @@ def __init__(self, model_conf, workdir: Path, log: bool=False, restart: bool=Fal
# 0 correctly since not all basin params can be loaded.
# There are probably some similar issues with explicit and independent, since they have
# similar data semantics
workdirs = list(Path.glob(workdir, model_conf['type']+"_*_worker"))
workdirs = list(Path.glob(workdir, ngen_model.type+"_*_worker"))
if len(workdirs) > 1:
print("More than one existing workdir, cannot restart")
elif len(workdirs) == 1:
self._job = JobMeta(model_conf['type'], workdir, workdirs[0], log=log)
self._job = JobMeta(ngen_model.type, workdir, workdirs[0], log=log)

if self._job is None:
self._job = JobMeta(model_conf['type'], workdir, log=log)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These steps are minutely important to the use of the Agent, and I don't think they can be dropped...The agent manipulates the configs to ensure root/workdirs are set appropriately under the automatically created agent working dirs.

Copy link
Member Author

@aaraney aaraney Aug 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think they are dropped? I was just pulling them from the pydantic model instance now instead of the dictionary. Probably missing something thought.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this comment ended up being unclear, it was intended for lines 81-83 where workdir and binary are configured relative to the Agent's dynamically created workdir. These are critical for correct operation of agents especially in PSO where multiple agents are running simultaneously.

As noted in disscussion with @aaraney the current resolved_binary isn't actually working as intended, and is its own bug which can be resolved in this context.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #167 to track this.

resolved_binary = Path(model_conf['binary']).resolve()
model_conf['workdir'] = self.job.workdir
self._model = Model(model=model_conf, binary=resolved_binary)
self._job = JobMeta(ngen_model.type, workdir, log=log)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are still missing the updated model workdir. An agent has a job which nests information and job/model files under a dynamically created workdir job.workdir. Any model instance associated with that job must have its workdir aligned with the job.workdir, so we need to update the model reference like such

Suggested change
self._job = JobMeta(ngen_model.type, workdir, log=log)
self._job = JobMeta(ngen_model.type, workdir, log=log)
ngen_model.workdir = self.job.workdir

ngen_model.workdir = self.job.workdir
self._model.model.resolve_paths(self.job.workdir)

self._params = parameters
Expand Down Expand Up @@ -117,4 +120,4 @@ def duplicate(self) -> Agent:
data = self.model.__root__.copy(deep=True)
#return a new agent, which has a unique Model instance
#and its own Job/workspace
return Agent(data.dict(by_alias=True), self._workdir)
return Agent(data, self._workdir)
5 changes: 5 additions & 0 deletions python/ngen_cal/src/ngen/cal/ngen.py
Original file line number Diff line number Diff line change
Expand Up @@ -595,6 +595,11 @@ def get_binary(self) -> str:
return self.__root__.get_binary()
def update_config(self, *args, **kwargs):
return self.__root__.update_config(*args, **kwargs)

def unwrap(self) -> NgenBase:
"""convenience method that returns the underlying __root__ instance"""
return self.__root__

#proxy methods for model
@property
def adjustables(self):
Expand Down
9 changes: 5 additions & 4 deletions python/ngen_cal/tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import json
import pandas as pd # type: ignore
import geopandas as gpd # type: ignore
from ngen.cal.configuration import General
from ngen.cal.configuration import General, Model
from ngen.cal.ngen import Ngen
from ngen.cal.meta import JobMeta
from ngen.cal.calibration_cathment import CalibrationCatchment
Expand Down Expand Up @@ -104,9 +104,10 @@ def meta(ngen_config, general_config, mocker) -> Generator[JobMeta, None, None]:
yield m

@pytest.fixture
def agent(ngen_config, general_config) -> Generator['Agent', None, None]:
a = Agent(ngen_config.__root__.dict(), general_config.workdir, general_config.log)
yield a
def agent(ngen_config, general_config) -> Agent:
model = Model(model=ngen_config)
a = Agent(model, general_config.workdir, general_config.log)
return a

@pytest.fixture
def eval(ngen_config) -> Generator[EvaluationOptions, None, None]:
Expand Down
6 changes: 6 additions & 0 deletions python/ngen_cal/tests/test_plugin_system.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
from hypy.nexus import Nexus

from ngen.cal.configuration import General
from ngen.cal.model import ModelExec
from pathlib import Path


def test_setup_plugin_manager():
Expand Down Expand Up @@ -74,6 +76,10 @@ def ngen_cal_start(self) -> None:
def ngen_cal_finish(self) -> None:
"""Called after exiting the calibration loop."""

@hookimpl
def ngen_cal_model_configure(self, config: ModelExec) -> None:
"""Test model model configure plugin"""

@hookimpl
def ngen_cal_model_output(self) -> None:
"""Test model output plugin"""
Expand Down
Loading