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

Conversation

aaraney
Copy link
Member

@aaraney aaraney commented Aug 9, 2024

Add model hook: ngen_cal_model_configure hook that mirrors the existing ngen_cal_configure hook.

    @hookspec
    def ngen_cal_model_configure(self, config: ngen.cal.model.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.
        """

Additions

  • ngen_cal_model_configure model hook; Called before calibration begins. This allow plugins to perform initial configuration.
  • unwrap method added to ngen.cal.ngen.Ngen. Returns the instance's __root__ attribute.

Changes

  • Agent now takes a Model instance as a parameter. Previously, the raw config was passed.

@aaraney aaraney added the ngen.cal Related to ngen.cal package label Aug 9, 2024
@aaraney aaraney self-assigned this Aug 9, 2024
@aaraney aaraney changed the title Add model config plugin feat: ngen_cal_model_configure model hook Aug 9, 2024
@aaraney aaraney marked this pull request as draft August 9, 2024 20:55
@aaraney aaraney marked this pull request as ready for review August 9, 2024 21:00
Copy link
Member

@hellkite500 hellkite500 left a comment

Choose a reason for hiding this comment

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

The change in behavior of the Agent and its interaction with the model config is quite likely to break.

python/ngen_cal/src/ngen/cal/agent.py Outdated Show resolved Hide resolved
python/ngen_cal/src/ngen/cal/agent.py Outdated Show resolved Hide resolved

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

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

print(_loaded_plugins(plugin_manager))

# setup plugins
plugin_manager.hook.ngen_cal_configure(config=general)
model_inner._plugin_manager.hook.ngen_cal_model_configure(config=model_inner)
Copy link
Member

Choose a reason for hiding this comment

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

An agent effectively side effects the Model it associates with, particularly setting the workdir for that model run. Because of this, we probably need to initialize the Agent before passing the Model to anything else, including plugins.

Suggested change
model_inner._plugin_manager.hook.ngen_cal_model_configure(config=model_inner)
# Initialize the starting agent
# WARNING: Agent sets the model workdir, so initialize it before using the model object for anything that may require an accurate workdir
agent = Agent(model, general.workdir, general.log, general.restart, general.strategy.parameters)
model_inner._plugin_manager.hook.ngen_cal_model_configure(config=model_inner)

Comment on lines +69 to +71

# Initialize the starting agent
agent = Agent(model, general.workdir, general.log, general.restart, general.strategy.parameters)
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)

aaraney and others added 2 commits August 15, 2024 14:38
Agent mutates the model config; If ngen_cal_model_configure is called
before the Agent is initialized, it won't get the final view of the
config before calibration.

Co-authored-by: Nels <[email protected]>
@hellkite500 hellkite500 merged commit aab8cf8 into NOAA-OWP:master Aug 15, 2024
12 checks passed
@aaraney aaraney deleted the add-model-config-plugin branch August 15, 2024 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ngen.cal Related to ngen.cal package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants