From 0a042fb07084c6fefaf84feb8d23a7c3a4deff4d Mon Sep 17 00:00:00 2001 From: Jasha <8935917+Jasha10@users.noreply.github.com> Date: Tue, 11 Jan 2022 02:15:44 -0600 Subject: [PATCH 1/5] remove deprecated Plugin.setup arg --- hydra/core/plugins.py | 81 ++++--------------------------------------- 1 file changed, 7 insertions(+), 74 deletions(-) diff --git a/hydra/core/plugins.py b/hydra/core/plugins.py index b22d6820e12..20425d7b210 100644 --- a/hydra/core/plugins.py +++ b/hydra/core/plugins.py @@ -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 @@ -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, *, @@ -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 From f2d2c2c85c6fe9c2fdc04ba0fe944d960a39213a Mon Sep 17 00:00:00 2001 From: Jasha <8935917+Jasha10@users.noreply.github.com> Date: Tue, 11 Jan 2022 02:41:44 -0600 Subject: [PATCH 2/5] update tests --- tests/test_hydra_context_warnings.py | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/tests/test_hydra_context_warnings.py b/tests/test_hydra_context_warnings.py index ead9b44e82b..e4080fea093 100644 --- a/tests/test_hydra_context_warnings.py +++ b/tests/test_hydra_context_warnings.py @@ -5,7 +5,7 @@ from unittest.mock import Mock from omegaconf import DictConfig, OmegaConf -from pytest import mark, warns +from pytest import mark, raises, warns from hydra import TaskFunction from hydra._internal.callbacks import Callbacks @@ -73,19 +73,14 @@ 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)): + classname = type(plugin).__name__ + msg = f"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( From fa0df8af750b5a220c4c1ac25dfbc35a66318b55 Mon Sep 17 00:00:00 2001 From: Jasha <8935917+Jasha10@users.noreply.github.com> Date: Tue, 11 Jan 2022 02:55:31 -0600 Subject: [PATCH 3/5] lint --- tests/test_hydra_context_warnings.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/test_hydra_context_warnings.py b/tests/test_hydra_context_warnings.py index e4080fea093..ee66b3b7247 100644 --- a/tests/test_hydra_context_warnings.py +++ b/tests/test_hydra_context_warnings.py @@ -73,8 +73,7 @@ def test_setup_plugins( monkeypatch.setattr(Plugins, "check_usage", lambda _: None) monkeypatch.setattr(plugin_instance, "_instantiate", lambda _: plugin) - classname = type(plugin).__name__ - msg = f"setup() got an unexpected keyword argument 'hydra_context'" + msg = "setup() got an unexpected keyword argument 'hydra_context'" with raises(TypeError, match=re.escape(msg)): if isinstance(plugin, Launcher): Plugins.instance().instantiate_launcher( From 69c816e11dcc1f3147ad370e60dfbbecc6366631 Mon Sep 17 00:00:00 2001 From: Jasha <8935917+Jasha10@users.noreply.github.com> Date: Thu, 13 Jan 2022 04:01:55 -0600 Subject: [PATCH 4/5] `hydra_context` arg now required by `run_job` --- hydra/core/utils.py | 30 ++++++++++------------------ tests/test_hydra_context_warnings.py | 11 +++++----- 2 files changed, 15 insertions(+), 26 deletions(-) diff --git a/hydra/core/utils.py b/hydra/core/utils.py index 7eb66746d50..1ec3f4dc085 100644 --- a/hydra/core/utils.py +++ b/hydra/core/utils.py @@ -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 @@ -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__) @@ -85,25 +82,17 @@ 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( @@ -111,10 +100,11 @@ def run_job( 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 diff --git a/tests/test_hydra_context_warnings.py b/tests/test_hydra_context_warnings.py index ee66b3b7247..6d5f0e97fdf 100644 --- a/tests/test_hydra_context_warnings.py +++ b/tests/test_hydra_context_warnings.py @@ -5,7 +5,7 @@ from unittest.mock import Mock from omegaconf import DictConfig, OmegaConf -from pytest import mark, raises, warns +from pytest import mark, raises from hydra import TaskFunction from hydra._internal.callbacks import Callbacks @@ -13,7 +13,7 @@ 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 @@ -93,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) From 624e5c2642fd6320bb183b130ab2a2dc4cede4bd Mon Sep 17 00:00:00 2001 From: Jasha <8935917+Jasha10@users.noreply.github.com> Date: Thu, 13 Jan 2022 04:02:54 -0600 Subject: [PATCH 5/5] add news fragment --- news/1953.maintenance | 1 + 1 file changed, 1 insertion(+) create mode 100644 news/1953.maintenance diff --git a/news/1953.maintenance b/news/1953.maintenance new file mode 100644 index 00000000000..f8c33bd78f8 --- /dev/null +++ b/news/1953.maintenance @@ -0,0 +1 @@ +Remove support for deprecated arg `config_loader` to Plugin.setup, and update signature of `run_job` to require `hydra_context`.