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

Remove deprecated handling of old Plugin.setup signature #1953

Merged
merged 5 commits into from
Jan 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 7 additions & 74 deletions hydra/core/plugins.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,12 @@
import warnings
from collections import defaultdict
from dataclasses import dataclass, field
from inspect import signature
from textwrap import dedent
from timeit import default_timer as timer
from typing import Any, Dict, List, Optional, Tuple, Type

from omegaconf import DictConfig

from hydra._internal.callbacks import Callbacks
from hydra._internal.deprecation_warning import deprecation_warning
from hydra._internal.sources_registry import SourcesRegistry
from hydra.core.config_loader import ConfigLoader
from hydra.core.singleton import Singleton
from hydra.plugins.completion_plugin import CompletionPlugin
from hydra.plugins.config_source import ConfigSource
Expand Down Expand Up @@ -108,59 +103,6 @@ def is_in_toplevel_plugins_module(clazz: str) -> bool:
"hydra._internal.core_plugins."
)

@staticmethod
def _setup_plugin(
plugin: Any,
task_function: TaskFunction,
config: DictConfig,
config_loader: Optional[ConfigLoader] = None,
hydra_context: Optional[HydraContext] = None,
) -> Any:
"""
With HydraContext introduced in #1581, we need to set up the plugins in a way
that's compatible with both Hydra 1.0 and Hydra 1.1 syntax.
This method should be deleted in the next major release.
"""
assert isinstance(plugin, Sweeper) or isinstance(plugin, Launcher)
assert (
config_loader is not None or hydra_context is not None
), "config_loader and hydra_context cannot both be None"

param_keys = signature(plugin.setup).parameters.keys()

if "hydra_context" not in param_keys:
# DEPRECATED: remove in 1.2
# hydra_context will be required in 1.2
deprecation_warning(
message=dedent(
"""
Plugin's setup() signature has changed in Hydra 1.1.
Support for the old style will be removed in Hydra 1.2.
For more info, check https://github.com/facebookresearch/hydra/pull/1581."""
),
)
config_loader = (
config_loader
if config_loader is not None
else hydra_context.config_loader # type: ignore
)
plugin.setup( # type: ignore
config=config,
config_loader=config_loader,
task_function=task_function,
)
else:
if hydra_context is None:
# hydra_context could be None when an incompatible Sweeper instantiates a compatible Launcher
assert config_loader is not None
hydra_context = HydraContext(
config_loader=config_loader, callbacks=Callbacks()
)
plugin.setup(
config=config, hydra_context=hydra_context, task_function=task_function
)
return plugin

def instantiate_sweeper(
self,
*,
Expand All @@ -172,36 +114,27 @@ def instantiate_sweeper(
if config.hydra.sweeper is None:
raise RuntimeError("Hydra sweeper is not configured")
sweeper = self._instantiate(config.hydra.sweeper)
sweeper = self._setup_plugin(
plugin=sweeper,
task_function=task_function,
config=config,
config_loader=None,
hydra_context=hydra_context,
)
assert isinstance(sweeper, Sweeper)
sweeper.setup(
hydra_context=hydra_context, task_function=task_function, config=config
)
return sweeper

def instantiate_launcher(
self,
hydra_context: HydraContext,
task_function: TaskFunction,
config: DictConfig,
config_loader: Optional[ConfigLoader] = None,
hydra_context: Optional[HydraContext] = None,
) -> Launcher:
Plugins.check_usage(self)
if config.hydra.launcher is None:
raise RuntimeError("Hydra launcher is not configured")

launcher = self._instantiate(config.hydra.launcher)
launcher = self._setup_plugin(
plugin=launcher,
config=config,
task_function=task_function,
config_loader=config_loader,
hydra_context=hydra_context,
)
assert isinstance(launcher, Launcher)
launcher.setup(
hydra_context=hydra_context, task_function=task_function, config=config
)
return launcher

@staticmethod
Expand Down
30 changes: 10 additions & 20 deletions hydra/core/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
from os.path import splitext
from pathlib import Path
from textwrap import dedent
from typing import TYPE_CHECKING, Any, Dict, Optional, Sequence, Union, cast
from typing import Any, Dict, Optional, Sequence, Union, cast

from omegaconf import DictConfig, OmegaConf, open_dict, read_write

Expand All @@ -20,9 +20,6 @@
from hydra.core.singleton import Singleton
from hydra.types import HydraContext, TaskFunction

if TYPE_CHECKING:
from hydra._internal.callbacks import Callbacks

log = logging.getLogger(__name__)


Expand Down Expand Up @@ -85,36 +82,29 @@ def filter_overrides(overrides: Sequence[str]) -> Sequence[str]:
return [x for x in overrides if not x.startswith("hydra.")]


def _get_callbacks_for_run_job(hydra_context: Optional[HydraContext]) -> "Callbacks":
def _check_hydra_context(hydra_context: Optional[HydraContext]) -> None:
if hydra_context is None:
# DEPRECATED: remove in 1.2
# hydra_context will be required in 1.2
deprecation_warning(
message=dedent(
# hydra_context is required as of Hydra 1.2.
# We can remove this check in Hydra 1.3.
raise TypeError(
dedent(
"""
run_job's signature has changed in Hydra 1.1. Please pass in hydra_context.
Support for the old style will be removed in Hydra 1.2.
run_job's signature has changed: the `hydra_context` arg is now required.
For more info, check https://github.com/facebookresearch/hydra/pull/1581."""
),
)
from hydra._internal.callbacks import Callbacks

callbacks = Callbacks()
else:
callbacks = hydra_context.callbacks

return callbacks


def run_job(
task_function: TaskFunction,
config: DictConfig,
job_dir_key: str,
job_subdir_key: Optional[str],
hydra_context: HydraContext,
configure_logging: bool = True,
hydra_context: Optional[HydraContext] = None,
) -> "JobReturn":
callbacks = _get_callbacks_for_run_job(hydra_context)
_check_hydra_context(hydra_context)
callbacks = hydra_context.callbacks

old_cwd = os.getcwd()
orig_hydra_cfg = HydraConfig.instance().cfg
Expand Down
1 change: 1 addition & 0 deletions news/1953.maintenance
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Remove support for deprecated arg `config_loader` to Plugin.setup, and update signature of `run_job` to require `hydra_context`.
23 changes: 8 additions & 15 deletions tests/test_hydra_context_warnings.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,15 @@
from unittest.mock import Mock

from omegaconf import DictConfig, OmegaConf
from pytest import mark, warns
from pytest import mark, raises

from hydra import TaskFunction
from hydra._internal.callbacks import Callbacks
from hydra._internal.config_loader_impl import ConfigLoaderImpl
from hydra._internal.utils import create_config_search_path
from hydra.core.config_loader import ConfigLoader
from hydra.core.plugins import Plugins
from hydra.core.utils import JobReturn, _get_callbacks_for_run_job
from hydra.core.utils import JobReturn, _check_hydra_context
from hydra.plugins.launcher import Launcher
from hydra.plugins.sweeper import Sweeper
from hydra.test_utils.test_utils import chdir_hydra_root
Expand Down Expand Up @@ -73,19 +73,13 @@ def test_setup_plugins(
monkeypatch.setattr(Plugins, "check_usage", lambda _: None)
monkeypatch.setattr(plugin_instance, "_instantiate", lambda _: plugin)

msg = dedent(
"""
Plugin's setup() signature has changed in Hydra 1.1.
Support for the old style will be removed in Hydra 1.2.
For more info, check https://github.com/facebookresearch/hydra/pull/1581."""
)
with warns(expected_warning=UserWarning, match=re.escape(msg)):
msg = "setup() got an unexpected keyword argument 'hydra_context'"
with raises(TypeError, match=re.escape(msg)):
if isinstance(plugin, Launcher):
Plugins.instance().instantiate_launcher(
hydra_context=hydra_context,
task_function=task_function,
config=config,
config_loader=config_loader,
hydra_context=hydra_context,
)
else:
Plugins.instance().instantiate_sweeper(
Expand All @@ -99,9 +93,8 @@ def test_run_job() -> None:
hydra_context = None
msg = dedent(
"""
run_job's signature has changed in Hydra 1.1. Please pass in hydra_context.
Support for the old style will be removed in Hydra 1.2.
run_job's signature has changed: the `hydra_context` arg is now required.
For more info, check https://github.com/facebookresearch/hydra/pull/1581."""
)
with warns(expected_warning=UserWarning, match=msg):
_get_callbacks_for_run_job(hydra_context)
with raises(TypeError, match=msg):
_check_hydra_context(hydra_context)