[RLlib] Fix connector registry #30635
Conversation
kouroshHakha
requested review from
gjoliver,
avnishn,
ArturNiederfahrenhorst,
smorad,
maxpumperla and
krfricke
as code owners
|
@gjoliver Please review, I have to disable connector in algorithm_config anyway, so I'll wait for your feedback and then flip the switch for merge. |
rllib/connectors/__init__.py
Outdated
|
|
||
|
|
||
| @PublicAPI(stability="alpha") | ||
| def register_connector(name: str, cls: Connector): |
There was a problem hiding this comment.
define these 2 in registry.py and just import here?
There was a problem hiding this comment.
Tried that, it gave me circular dependency issues :( I solved circular dependency by this. Basically the problem is that pipleineconnectors use register_connector and get_connector. I cannot import them in the registry.py if registry.py also includes register_connector. The register_connector API has to be imported before both of these connector apis.
There was a problem hiding this comment.
I thought the reason to use a registry is to break this circular dependency.
so why do we move these 2 out of connector.py?
rllib/__init__.py
Outdated
| @@ -58,6 +61,7 @@ def setup(self, config): | |||
| register_trainable(alias, _see_contrib(alias)) | |||
|
|
|||
|
|
|||
| _register_all() | |||
There was a problem hiding this comment.
looks a bit suspicious. why didn't we have to register anything before this PR?
There was a problem hiding this comment.
We have explicitly called _register_all() in places where it was needed. for example in the tune_restore_test() we explicitly call this in the tear_down() function to reimport all the classes after the tune actor dies. For some reason that does not work for the connectors. We could have also called register_all when importing algorithm.py but I don't think that's the right place to call register_all. Basically the logic is that as soon as you import rllib all the namespaces with rllib should be recognizable globally.
… the global registry is lost. Signed-off-by: Jun Gong <jungong@anyscale.com>
Signed-off-by: Jun Gong <jungong@anyscale.com>
gjoliver
force-pushed
the
fix-connector-registry
branch
from
e6046ee to
5786282
Compare
|
Tests look good when I flip enable_connectors to True. Reverting the flag change now. |
Signed-off-by: Jun Gong <jungong@anyscale.com>
| """ | ||
| for module in ALL_CONNECTORS: | ||
| if module in sys.modules: | ||
| importlib.reload(sys.modules[module]) |
There was a problem hiding this comment.
nice. I learned something new :)
Introduce API to force re-register all the connectors in case the global registry is lost. Signed-off-by: Jun Gong <jungong@anyscale.com>
Introduce API to force re-register all the connectors in case the global registry is lost. Signed-off-by: Jun Gong <jungong@anyscale.com> Signed-off-by: Weichen Xu <weichen.xu@databricks.com>
Introduce API to force re-register all the connectors in case the global registry is lost. Signed-off-by: Jun Gong <jungong@anyscale.com> Signed-off-by: tmynn <hovhannes.tamoyan@gmail.com>
Nice!
Why are these changes needed?
This fixes test_tune_restore with connectors enabled.
Related issue number
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.