Issue warning on outdated API usage #1603

jieru-hu · 2021-05-03T22:55:14Z

running with an outdated sweeper

$ python /Users/jieru/workspace/hydra-fork/hydra/plugins/hydra_ax_sweeper/example/banana.py -m
/Users/jieru/workspace/hydra-fork/hydra/hydra/core/plugins.py:127: UserWarning:


        Plugin's setup() signature has changed in Hydra 1.1.
        Support for the old style will be removed in Hydra 1.2.


[2021-05-03 18:05:09,476][HYDRA] AxSweeper is optimizing the following parameters: 
banana.x: range=[-5, 5]
banana.y: range=[-5.0, 10.1]
[INFO 05-03 18:05:09] ax.service.utils.instantiation: Inferred value type of ParameterType.INT for parameter banana.x. If that is not the expected value type, you can explicity specify 'value_type' ('int', 'float', 'bool' or 'str') in parameter dict.
[INFO 05-03 18:05:09] ax.service.utils.instantiation: Inferred value type of ParameterType.FLOAT for parameter banana.y. If that is not the expected value type, you can explicity specify 'value_type' ('int', 'float', 'bool' or 'str') in parameter dict.
[INFO 05-03 18:05:09] ax.modelbridge.dispatch_utils: Using Bayesian Optimization generation strategy: GenerationStrategy(name='Sobol+GPEI', steps=[Sobol for 5 trials, GPEI for subsequent trials]). Iterations after 5 will take longer to generate due to  model-fitting.
[2021-05-03 18:05:09,575][HYDRA] AxSweeper is launching 5 jobs
[2021-05-03 18:05:10,054][HYDRA] Launching 5 jobs locally
[2021-05-03 18:05:10,054][HYDRA]        #0 : banana.x=-2 banana.y=-1.4334993640892209
[2021-05-03 18:05:10,190][__main__][INFO] - Banana_Function(x=-2, y=-1.4334993640892209)=2961.2915339557962
[2021-05-03 18:05:10,191][HYDRA]        #1 : banana.x=3 banana.y=0.25573451602831465
[2021-05-03 18:05:10,320][__main__][INFO] - Banana_Function(x=3, y=0.25573451602831465)=7650.217885417856
[2021-05-03 18:05:10,321][HYDRA]        #2 : banana.x=-3 banana.y=6.488616014365107
[2021-05-03 18:05:10,433][__main__][INFO] - Banana_Function(x=-3, y=6.488616014365107)=646.7049523303401
[2021-05-03 18:05:10,434][HYDRA]        #3 : banana.x=-4 banana.y=-1.6604875128716232
[2021-05-03 18:05:10,567][__main__][INFO] - Banana_Function(x=-4, y=-1.6604875128716232)=31214.28191922945
[2021-05-03 18:05:10,568][HYDRA]        #4 : banana.x=-5 banana.y=8.075565069634466
[2021-05-03 18:05:10,690][__main__][INFO] - Banana_Function(x=-5, y=8.075565069634466)=28679.649771217708
[2021-05-03 18:05:10,754][HYDRA] New best value: 646.7049523303401, best parameters: {'banana.x': -3, 'banana.y': 6.488616014365107}
[2021-05-03 18:05:12,843][HYDRA] AxSweeper is launching 3 jobs

Running with outdated Launcher

$ python /Users/jieru/workspace/hydra-fork/hydra/plugins/hydra_joblib_launcher/example/my_app.py -m 
/Users/jieru/workspace/hydra-fork/hydra/plugins/hydra_joblib_launcher/example/my_app.py:12: UserWarning: 
config_path is not specified in @hydra.main().
See https://hydra.cc/docs/next/upgrades/1.0_to_1.1/changes_to_hydra_main_config_path for more information.
  @hydra.main(config_name="config")
/Users/jieru/workspace/hydra-fork/hydra/hydra/core/plugins.py:127: UserWarning: 
        Plugin's setup() signature has changed in Hydra 1.1.
        Support for the old style will be removed in Hydra 1.2.

  warnings.warn(
[2021-05-03 18:06:37,759][HYDRA] Joblib.Parallel(n_jobs=10,backend=loky,prefer=processes,require=None,verbose=0,timeout=None,pre_dispatch=2*n_jobs,batch_size=auto,temp_folder=None,max_nbytes=None,mmap_mode=r) is launching 1 jobs
[2021-05-03 18:06:37,759][HYDRA] Launching jobs, sweep output dir : multirun/2021-05-03/18-06-37
[2021-05-03 18:06:37,759][HYDRA]        #0 : 
/Users/jieru/workspace/hydra-fork/hydra/hydra/core/utils.py:92: UserWarning: 
        run_job's signature has changed in Hydra 1.1.
        Support for the old style will be removed in Hydra 1.2.

  warnings.warn(
[2021-05-03 18:06:38,504][__main__][INFO] - Process ID 3429 executing task 1 ...

lgtm-com · 2021-05-04T00:09:05Z

This pull request introduces 2 alerts when merging 4c73fab into 901fb39 - view on LGTM.com

new alerts:

1 for Signature mismatch in overriding method
1 for Module is imported with 'import' and 'import from'

omry · 2021-05-04T00:29:38Z

I approved #1581, can you land and rebase? I don't want to review it again in this diff.

jieru-hu · 2021-05-04T00:33:44Z

I approved #1581, can you land and rebase? I don't want to review it again in this diff.

Thanks! Yea, once it's ready I will change the state from DRAFT to Ready to review :)

lgtm-com · 2021-05-04T01:24:58Z

This pull request introduces 2 alerts when merging 6f2cca1 into c6397d2 - view on LGTM.com

new alerts:

1 for Signature mismatch in overriding method
1 for Module is imported with 'import' and 'import from'

jieru-hu · 2021-05-04T01:31:39Z

rebase, also updated the issue description with what the warnings looks like for outdated Plugin.

lgtm-com · 2021-05-04T01:38:51Z

This pull request introduces 2 alerts when merging 78913fe into c6397d2 - view on LGTM.com

new alerts:

1 for Signature mismatch in overriding method
1 for Module is imported with 'import' and 'import from'

lgtm-com · 2021-05-04T01:58:52Z

This pull request introduces 2 alerts when merging 12049bd into c6397d2 - view on LGTM.com

new alerts:

1 for Signature mismatch in overriding method
1 for Module is imported with 'import' and 'import from'

lgtm-com · 2021-05-04T17:42:07Z

This pull request introduces 2 alerts when merging fbdbfc4 into c6397d2 - view on LGTM.com

new alerts:

1 for Signature mismatch in overriding method
1 for Module is imported with 'import' and 'import from'

hydra/_internal/core_plugins/basic_launcher.py

hydra/core/plugins.py

lgtm-com · 2021-05-04T21:37:14Z

This pull request introduces 1 alert when merging d2a4213 into 43cc401 - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'

omry · 2021-05-05T00:24:02Z

hydra/core/plugins.py

+
+        param_keys = signature(plugin.setup).parameters.keys()
+
+        if "config_loader" in param_keys:


More directly is better:

if "hydra_context" not in param_keys: # backward compatibility else: # new flow

hydra/core/utils.py

hydra/plugins/sweeper.py

omry · 2021-05-05T00:45:31Z

tests/test_hydra_context_warnings.py

+def test_setup_plugin(
+    config_loader: Optional[ConfigLoader],
+    hydra_context: Optional[HydraContext],
+    setup_method: Any,
+    expected: Any,
+) -> None:
+    plugin = Mock(spec=Sweeper)
+    plugin.setup = setup_method
+    config = Mock(spec=DictConfig)
+    task_function = Mock(spec=TaskFunction)
+    msg = (
+        "\n"
+        "\tPlugin's setup() signature has changed in Hydra 1.1.\n"
+        "\tSupport for the old style will be removed in Hydra 1.2.\n"
+        "\tFor more info, check https://github.com/facebookresearch/hydra/pull/1581.\n"
+    )
+    if expected is None:
+        plugins._setup_plugin(
+            plugin, config, task_function, config_loader, hydra_context
+        )
+    elif isinstance(expected, UserWarning):
+        with warns(expected_warning=UserWarning, match=re.escape(msg)):
+            plugins._setup_plugin(
+                plugin, task_function, config, config_loader, hydra_context
+            )
+    else:
+        with raises(expected):
+            plugins._setup_plugin(
+                plugin, config, task_function, config_loader, hydra_context
+            )


This seems very elaborate.
Why not test it more directly by using Plugins.instantiate_sweeper/instantiate_launcher methods on a deliberately incompatible plugin?

Tests updated!

lgtm-com · 2021-05-05T21:37:10Z

This pull request introduces 1 alert when merging 2d4caf0 into ed267e0 - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'

hydra/core/plugins.py

omry · 2021-05-06T06:29:36Z

hydra/core/plugins.py

+            if hydra_context is None:
+                assert config_loader is not None
+                hydra_context = HydraContext(
+                    config_loader=config_loader, callbacks=Callbacks()
+                )


when is hydra_context none here?
If this is just to support tests, have them instantiate the HydraContext.

hydra_context could be None in the case of an outdated Sweeper tried to instantiate an update to date Launcher.

I've added a comment here.

u$ python /Users/jieru/workspace/hydra-fork/hydra/plugins/hydra_ax_sweeper/example/banana.py -m /Users/jieru/workspace/hydra-fork/hydra/hydra/core/plugins.py:131: UserWarning: 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. [2021-05-06 10:38:30,027][HYDRA] AxSweeper is optimizing the following parameters: banana.x: range=[-5, 5] banana.y: range=[-5.0, 10.1] [INFO 05-06 10:38:30] ax.service.utils.instantiation: Inferred value type of ParameterType.INT for parameter banana.x. If that is not the expected value type, you can explicity specify 'value_type' ('int', 'float', 'bool' or 'str') in parameter dict. [INFO 05-06 10:38:30] ax.service.utils.instantiation: Inferred value type of ParameterType.FLOAT for parameter banana.y. If that is not the expected value type, you can explicity specify 'value_type' ('int', 'float', 'bool' or 'str') in parameter dict. [INFO 05-06 10:38:30] ax.modelbridge.dispatch_utils: Using Bayesian Optimization generation strategy: GenerationStrategy(name='Sobol+GPEI', steps=[Sobol for 5 trials, GPEI for subsequent trials]). Iterations after 5 will take longer to generate due to model-fitting. [2021-05-06 10:38:30,130][HYDRA] AxSweeper is launching 5 jobs Traceback (most recent call last): File "/Users/jieru/workspace/hydra-fork/hydra/hydra/_internal/utils.py", line 212, in run_and_report return func() File "/Users/jieru/workspace/hydra-fork/hydra/hydra/_internal/utils.py", line 379, in <lambda> lambda: hydra.multirun( File "/Users/jieru/workspace/hydra-fork/hydra/hydra/_internal/hydra.py", line 139, in multirun ret = sweeper.sweep(arguments=task_overrides) File "/Users/jieru/opt/anaconda3/envs/pytest38/lib/python3.8/site-packages/hydra_plugins/hydra_ax_sweeper/ax_sweeper.py", line 29, in sweep return self.sweeper.sweep(arguments) File "/Users/jieru/opt/anaconda3/envs/pytest38/lib/python3.8/site-packages/hydra_plugins/hydra_ax_sweeper/_core.py", line 180, in sweep self.sweep_over_batches( File "/Users/jieru/opt/anaconda3/envs/pytest38/lib/python3.8/site-packages/hydra_plugins/hydra_ax_sweeper/_core.py", line 217, in sweep_over_batches rets = self.launcher.launch( File "/Users/jieru/workspace/hydra-fork/hydra/hydra/_internal/core_plugins/basic_launcher.py", line 55, in launch assert self.hydra_context is not None AssertionError

lgtm-com · 2021-05-06T18:04:49Z

This pull request introduces 1 alert when merging aa98b80 into cf23e4d - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'

hydra/core/utils.py

omry · 2021-05-06T18:52:56Z

hydra/core/utils.py

+            run_job's 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.


Suggested change

run_job's 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.

run_job's signature has changed in Hydra 1.1. Please pass hydra_context.

Support for the old style will be removed in Hydra 1.2.

For more info, check https://github.com/facebookresearch/hydra/pull/1581.

The info link #1581 should contain some TLDR for what people should do to migrate. Sending them to read a design doc does not feel right.
This can also include links to the updated example plugins.

Thanks! I've updated the PR's description to provide more info
#1581

pro tip:
If you are linking to a specific line in master, that link will become wrong when the file changes later.
You can fix it by linking to a specific revision: while you are on the link to master, hit y and the browser link will change.

nice!! I've fixed the links in #1581

hydra/core/utils.py

lgtm-com · 2021-05-06T19:31:47Z

This pull request introduces 1 alert when merging 8a5da04 into cf23e4d - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'

omry

I don't know why, but somehow I can't reply to your other comments.

omry · 2021-05-06T19:50:23Z

hydra/core/plugins.py

+            )
+        else:
+            if hydra_context is None:
+                # hydra_context could be None when an incompatible Sweeper tried to compatible Launcher


Suggested change

# hydra_context could be None when an incompatible Sweeper tried to compatible Launcher

# hydra_context could be None when an incompatible Sweeper tried to use a compatible Launcher

lgtm-com · 2021-05-06T20:26:52Z

This pull request introduces 1 alert when merging b4c277f into cf23e4d - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'

omry

Looking good. see last nit.

omry · 2021-05-06T20:47:18Z

tests/test_hydra_context_warnings.py

+        """
+            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.
+            For more info, check https://github.com/facebookresearch/hydra/pull/1581.
+            """


nit: why is this not aligned? (it's not the only case).
Look at how I use dedent in other places.

Suggested change

"""

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.

For more info, check https://github.com/facebookresearch/hydra/pull/1581.

"""

"""

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.

For more info, check https://github.com/facebookresearch/hydra/pull/1581.

"""

black auto-formatted like that. I moved the """ to the end of the string which seems to have stopped black from moving the quotes around :)

lgtm-com · 2021-05-06T22:00:39Z

This pull request introduces 1 alert when merging b9508c6 into cf23e4d - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 3, 2021

jieru-hu changed the title ~~Warnings sig~~ Issue warning on outdated API usage May 3, 2021

jieru-hu force-pushed the warnings_sig branch from 4c73fab to 6f2cca1 Compare May 4, 2021 01:04

jieru-hu force-pushed the warnings_sig branch from 6f2cca1 to 78913fe Compare May 4, 2021 01:27

jieru-hu mentioned this pull request May 4, 2021

Document HydraContext change #1604

Closed

Issue warning on outdated API usage

12049bd

jieru-hu force-pushed the warnings_sig branch from 78913fe to 12049bd Compare May 4, 2021 01:39

jieru-hu marked this pull request as ready for review May 4, 2021 01:48

jieru-hu requested a review from omry May 4, 2021 01:48

Update warning message to include issue link

fbdbfc4

omry reviewed May 4, 2021

View reviewed changes

hydra/_internal/core_plugins/basic_launcher.py Outdated Show resolved Hide resolved

hydra/core/plugins.py Outdated Show resolved Hide resolved

remove BasicLauncher constraints

d2a4213

jieru-hu requested a review from omry May 4, 2021 23:48

omry reviewed May 5, 2021

View reviewed changes

fix tests

2d4caf0

jieru-hu requested a review from omry May 5, 2021 21:08

omry reviewed May 6, 2021

View reviewed changes

fix text dedent

aa98b80

jieru-hu requested a review from omry May 6, 2021 18:01

omry reviewed May 6, 2021

View reviewed changes

add comment

8a5da04

omry reviewed May 6, 2021

View reviewed changes

commit

b4c277f

jieru-hu requested a review from omry May 6, 2021 20:14

omry approved these changes May 6, 2021

View reviewed changes

fix formatting

b9508c6

jieru-hu merged commit f0fd13e into facebookresearch:master May 6, 2021

jieru-hu deleted the warnings_sig branch May 6, 2021 22:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue warning on outdated API usage #1603

Issue warning on outdated API usage #1603

jieru-hu commented May 3, 2021 •

edited

Loading

lgtm-com bot commented May 4, 2021

omry commented May 4, 2021

jieru-hu commented May 4, 2021

lgtm-com bot commented May 4, 2021

jieru-hu commented May 4, 2021

lgtm-com bot commented May 4, 2021

lgtm-com bot commented May 4, 2021

lgtm-com bot commented May 4, 2021

lgtm-com bot commented May 4, 2021

omry May 5, 2021

jieru-hu May 5, 2021

omry May 5, 2021

jieru-hu May 5, 2021

lgtm-com bot commented May 5, 2021

omry May 6, 2021

jieru-hu May 6, 2021 •

edited

Loading

omry May 6, 2021

lgtm-com bot commented May 6, 2021

omry May 6, 2021

omry May 6, 2021

jieru-hu May 6, 2021

omry May 6, 2021

jieru-hu May 6, 2021

lgtm-com bot commented May 6, 2021

omry left a comment

omry May 6, 2021

jieru-hu May 6, 2021

lgtm-com bot commented May 6, 2021

omry left a comment

omry May 6, 2021

jieru-hu May 6, 2021 •

edited

Loading

lgtm-com bot commented May 6, 2021


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

		if "config_loader" in param_keys:

	# hydra_context could be None when an incompatible Sweeper tried to compatible Launcher
	# hydra_context could be None when an incompatible Sweeper tried to use a compatible Launcher

Issue warning on outdated API usage #1603

Issue warning on outdated API usage #1603

Conversation

jieru-hu commented May 3, 2021 • edited Loading

running with an outdated sweeper

Running with outdated Launcher

lgtm-com bot commented May 4, 2021

omry commented May 4, 2021

jieru-hu commented May 4, 2021

lgtm-com bot commented May 4, 2021

jieru-hu commented May 4, 2021

lgtm-com bot commented May 4, 2021

lgtm-com bot commented May 4, 2021

lgtm-com bot commented May 4, 2021

lgtm-com bot commented May 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented May 5, 2021

Choose a reason for hiding this comment

jieru-hu May 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented May 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented May 6, 2021

omry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented May 6, 2021

omry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jieru-hu May 6, 2021 • edited Loading

Choose a reason for hiding this comment

lgtm-com bot commented May 6, 2021

jieru-hu commented May 3, 2021 •

edited

Loading

jieru-hu May 6, 2021 •

edited

Loading

jieru-hu May 6, 2021 •

edited

Loading