remove Plugins from Singleton get_state #1312

jieru-hu · 2021-01-20T01:54:43Z

Motivation

We do not need to pickle all of Singleton. change this to only pickle what is needed (JobRuntime and OmegaConf resolvers.)

This will also fix that the plugin integration tests fails if other hydra tools/plugins is isntalled locally as well.

Have you read the Contributing Guidelines on pull requests?

Yes/No

Test Plan

CI
locally

# install other plugin along with  ray launcher
FIX=1 PLUGINS=hydra_colorlog,hydra_ray_launcher  nox -s lint_plugins-3.8 test_plugins-3.8

Related Issues and PRs

(Is this PR part of a group of changes? Link the other relevant PRs and Issues here. Use https://help.github.com/en/articles/closing-issues-using-keywords for help on GitHub syntax)

omry · 2021-01-20T02:55:09Z

What I had in mind is that we:

fix get_state() to not return the the Plugins singleton state.
fix set_state() to call Plugins.instance() to (re)initialize the plugins.

omry · 2021-01-20T19:04:59Z

plugins/hydra_ray_launcher/hydra_plugins/hydra_ray_launcher/_remote_invoke.py

@@ -45,6 +46,7 @@ def launch_jobs(temp_dir: str) -> None:
                )
            setup_globals()
            Singleton.set_state(singleton_state)
+            Plugins.instance()


And this inside set_state()?

plugins/hydra_ray_launcher/hydra_plugins/hydra_ray_launcher/_core_aws.py

jieru-hu · 2021-01-20T19:06:05Z

What I had in mind is that we:

fix get_state() to not return the the Plugins singleton state.

fix set_state() to call Plugins.instance() to (re)initialize the plugins.

I see. This does seem better. I've fixed the PR to be along the same line of thoughts.

I'm not sure if you meant to actually change Singleton class. I've also tried that but ran into a circular imports issue. I didn't go further to make it work, since the issue I'm trying to fix here is still on only ray plugin level.

more details below,

diff --git a/hydra/core/singleton.py b/hydra/core/singleton.py
index d184a4e29..13bd5a9db 100644
--- a/hydra/core/singleton.py
+++ b/hydra/core/singleton.py
@@ -4,6 +4,8 @@ from typing import Any, Dict
 
 from omegaconf.basecontainer import BaseContainer
 
+from hydra.core.plugins import Plugins
+
 
 class Singleton(type):
     _instances: Dict[type, "Singleton"] = {}
@@ -18,6 +20,7 @@ class Singleton(type):
 
     @staticmethod
     def get_state() -> Any:
+        del Singleton.get_state().get("instances")[Plugins]
         return {
             "instances": Singleton._instances,
             "omegaconf_resolvers": deepcopy(BaseContainer._resolvers),
@@ -26,4 +29,5 @@ class Singleton(type):
     @staticmethod
     def set_state(state: Any) -> None:
         Singleton._instances = state["instances"]
+        Plugins.instance()
         BaseContainer._resolvers = deepcopy(state["omegaconf_resolvers"])



Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/Users/jieru/workspace/hydra-fork/hydra/hydra/__init__.py", line 5, in <module>
    from hydra import utils
  File "/Users/jieru/workspace/hydra-fork/hydra/hydra/utils.py", line 12, in <module>
    from hydra._internal.utils import (
  File "/Users/jieru/workspace/hydra-fork/hydra/hydra/_internal/utils.py", line 19, in <module>
    from hydra.core.utils import get_valid_filename, split_config_path
  File "/Users/jieru/workspace/hydra-fork/hydra/hydra/core/utils.py", line 17, in <module>
    from hydra.core.hydra_config import HydraConfig
  File "/Users/jieru/workspace/hydra-fork/hydra/hydra/core/hydra_config.py", line 6, in <module>
    from hydra.conf import HydraConf
  File "/Users/jieru/workspace/hydra-fork/hydra/hydra/conf/__init__.py", line 7, in <module>
    from hydra.core.config_store import ConfigStore
  File "/Users/jieru/workspace/hydra-fork/hydra/hydra/core/config_store.py", line 9, in <module>
    from hydra.core.singleton import Singleton
  File "/Users/jieru/workspace/hydra-fork/hydra/hydra/core/singleton.py", line 7, in <module>
    from hydra.core.plugins import Plugins
  File "/Users/jieru/workspace/hydra-fork/hydra/hydra/core/plugins.py", line 14, in <module>
    from hydra._internal.sources_registry import SourcesRegistry
  File "/Users/jieru/workspace/hydra-fork/hydra/hydra/_internal/sources_registry.py", line 4, in <module>
    from hydra.core.singleton import Singleton
ImportError: cannot import name 'Singleton' from partially initialized module 'hydra.core.singleton' (most likely due to a circular import) (/Users/jieru/workspace/hydra-fork/hydra/hydra/core/singleton.py)

jieru-hu · 2021-01-20T20:48:53Z

per offline discussion - move imports to local :)

hydra/core/singleton.py

jieru-hu · 2021-01-21T21:39:03Z

You are changing the actual singleton state, don't do that.
Make a copy of the map without the Plugins singleton.
The only reason this did not cause issues is that plugin interaction is currently only happening on startup, but this can change.

I see. Thanks! updated the PR per your comment, also added a unit test.

jieru-hu · 2021-01-22T16:59:02Z

rebase...

omry · 2021-01-22T20:21:19Z

hydra/core/singleton.py

+        try:
+            from hydra.core.plugins import Plugins
+
+            del instances[Plugins]
+        except KeyError:
+            pass


try:

instances.pop(Plugins, None)

hydra/core/singleton.py

This reverts commit bcd1627.

This reverts commit 0f4e58a.

hydra/core/singleton.py

omry · 2021-01-22T21:24:58Z

plugins/hydra_ray_launcher/tests/test_ray_aws_launcher.py

    chdir_hydra_root()
    plugin = "hydra_ray_launcher"
    os.chdir(Path("plugins") / plugin)


You can probably use monkeypatch to change the directory and restore automatically.

I didn't know monkeypatch could do that, I thought we usually do fixture?

https://github.com/facebookresearch/hydra/blob/master/tests/test_hydra.py#L1049-L1050

I see, thanks!

hydra/core/singleton.py

omry

Merge once CI passes.

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 Jan 20, 2021

jieru-hu force-pushed the ray_pickle branch from 21e36f2 to bcd1627 Compare January 20, 2021 01:56

jieru-hu marked this pull request as draft January 20, 2021 16:54

jieru-hu requested a review from omry January 20, 2021 19:02

omry reviewed Jan 20, 2021

View reviewed changes

jieru-hu marked this pull request as ready for review January 20, 2021 19:07

jieru-hu requested a review from omry January 20, 2021 19:07

jieru-hu marked this pull request as draft January 20, 2021 19:17

jieru-hu force-pushed the ray_pickle branch 2 times, most recently from dc6db48 to 2ebfea7 Compare January 20, 2021 19:38

jieru-hu marked this pull request as ready for review January 20, 2021 20:48

jieru-hu changed the title ~~remove pickling Singleton~~ remove Plugins from Singleton get_state Jan 20, 2021

omry requested changes Jan 21, 2021

View reviewed changes

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

jieru-hu force-pushed the ray_pickle branch from 8dd32d1 to 7a2ddc9 Compare January 21, 2021 20:55

jieru-hu requested a review from omry January 21, 2021 21:38

jieru-hu force-pushed the ray_pickle branch from 7a2ddc9 to 353af48 Compare January 22, 2021 16:56

omry requested changes Jan 22, 2021

View reviewed changes

jieru-hu added 7 commits January 22, 2021 12:49

remove pickling Singleton

008afe3

Revert "remove pickling Singleton"

d1ebe3b

This reverts commit bcd1627.

commit

eeb0701

Revert "commit"

49e3007

This reverts commit 0f4e58a.

local imports

03a0f7c

commit

eca48b9

create a copy before return

6706614

jieru-hu force-pushed the ray_pickle branch from 353af48 to 2247327 Compare January 22, 2021 21:07

address comments

c87ef5b

jieru-hu force-pushed the ray_pickle branch from 2247327 to c87ef5b Compare January 22, 2021 21:22

omry reviewed Jan 22, 2021

View reviewed changes

fix comments

f3f6377

jieru-hu requested a review from omry January 22, 2021 23:58

omry approved these changes Jan 23, 2021

View reviewed changes

jieru-hu merged commit f664c5e into facebookresearch:master Jan 23, 2021

jieru-hu deleted the ray_pickle branch January 23, 2021 00:38

jieru-hu added a commit to jieru-hu/hydra that referenced this pull request Jan 23, 2021

remove Plugins from Singleton get_state (facebookresearch#1312)

a9bf773

jieru-hu mentioned this pull request Jan 23, 2021

remove Plugins from Singleton get_state, fix ray launcher default key pair name #1330

Merged

jieru-hu added a commit to jieru-hu/hydra that referenced this pull request Jan 27, 2021

remove Plugins from Singleton get_state (facebookresearch#1312)

04d6b9f

jieru-hu added a commit that referenced this pull request Jan 27, 2021

remove Plugins from Singleton get_state (#1312)

49a3b8d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove Plugins from Singleton get_state #1312

remove Plugins from Singleton get_state #1312

jieru-hu commented Jan 20, 2021 •

edited

Loading

omry commented Jan 20, 2021

omry Jan 20, 2021

jieru-hu commented Jan 20, 2021

jieru-hu commented Jan 20, 2021

jieru-hu commented Jan 21, 2021

jieru-hu commented Jan 22, 2021

omry Jan 22, 2021

jieru-hu Jan 22, 2021

omry Jan 22, 2021

jieru-hu Jan 22, 2021

omry Jan 22, 2021

jieru-hu Jan 23, 2021

omry left a comment

remove Plugins from Singleton get_state #1312

remove Plugins from Singleton get_state #1312

Conversation

jieru-hu commented Jan 20, 2021 • edited Loading

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

Related Issues and PRs

omry commented Jan 20, 2021

omry Jan 20, 2021

Choose a reason for hiding this comment

jieru-hu commented Jan 20, 2021

jieru-hu commented Jan 20, 2021

jieru-hu commented Jan 21, 2021

jieru-hu commented Jan 22, 2021

omry Jan 22, 2021

Choose a reason for hiding this comment

jieru-hu Jan 22, 2021

Choose a reason for hiding this comment

omry Jan 22, 2021

Choose a reason for hiding this comment

jieru-hu Jan 22, 2021

Choose a reason for hiding this comment

omry Jan 22, 2021

Choose a reason for hiding this comment

jieru-hu Jan 23, 2021

Choose a reason for hiding this comment

omry left a comment

Choose a reason for hiding this comment

jieru-hu commented Jan 20, 2021 •

edited

Loading