[Prototype] [Core] Make expensive subpackage imports dynamic. #27658

clarkzinzow · 2022-08-08T19:14:16Z

Certain Ray subpackages are expensive to import, either due to their size, dependencies, or their import-time logic that must be executed. E.g. a change in a Datasets import caused a regression for the many_tasks nightly test, despite many_tasks only using Ray Core.

This PR delays the import of these expensive subpackages until attribute access, e.g. none of the Datasets code will be run until ray.data is accessed.

Related issue number

Closes #27606, closes #29557

Checks

I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

clarkzinzow · 2022-08-08T19:16:47Z

python/ray/__init__.py

+    def __getattr__(name: str):
+        if name in _subpackages:
+            return importlib.import_module("." + name, __name__)
+        raise AttributeError(f"module {__name__!r} has no attribute {name!r}")


Note that, similar to an object's __getattr__ method, this __getattr__ will only be called if normal attribute lookup (e.g. via module.__getattribute__) fails, so this will not run for any of the above subpackages that are eagerly imported and should only run for the packages in _subpackages (or non-existent module attributes, which will error anyway).

python/ray/__init__.py

clarkzinzow · 2022-08-08T19:24:51Z

python/ray/__init__.py

+    # TODO(Clark): Remove this one we drop Python 3.6 support.
+    from ray import autoscaler  # noqa: F401
+    from ray import data  # noqa: F401
+    from ray import workflow  # noqa: F401


We could implement something similar for Python 3.6 via a "replace module with class hack":

class module(types.ModuleType): __all__ = __all__ def __init__(self, orig_module: types.ModuleType): self._orig_module = orig_module def __getattr__(self, name: str): try: return getattr(self._orig_module, name) except AttributeError as e: if name in _subpackages: return importlib.import_module("." + name, self._orig_module.__name__) raise e from None sys.modules[__name__] = module(sys.modules[__name__])

But IMO this is more hacky than its worth since we're going to be dropping Python 3.6 support in the near-term.

stale · 2022-09-08T17:11:47Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

If you'd like to keep this open, just leave any comment, and the stale label will be removed.

stale · 2022-09-24T03:46:47Z

Hi again! The issue will be closed because there has been no more activity in the 14 days since the last message.

Please feel free to reopen or open a new issue if you'd still like it to be addressed.

Again, you can always ask for help on our discussion forum or Ray's public slack channel.

Thanks again for opening the issue!

rkooo567 · 2022-10-20T06:59:07Z

This one seems to be pretty good? What was the original reason why we decided to defer this PR?

rkooo567 · 2022-10-20T06:59:21Z

I'd love to merge it. What's the main concern of this approach?

clarkzinzow · 2022-10-20T07:05:47Z

@rkooo567 it was originally deferred because the many_tasks perf gain wasn't very significant (10-15% IIRC) and there are still some odd CI failures to investigate.

rkooo567

LGTM. Maybe we can just add minimal tests? Also the approach seems reasonable. Is there any concern on top of your head doing this?

rkooo567 · 2022-10-20T07:14:03Z

python/ray/__init__.py

 from ray import internal  # noqa: E402,F401
-from ray import util  # noqa: E402
+from ray import util  # noqa: E402,F401


Just OOC, why did it work before?

Not sure 🧐

python/ray/__init__.py

rkooo567 · 2022-10-20T07:17:35Z

python/ray/__init__.py


 del os
 del logging
+del sys


maybe just leave it as it is? A little concern there might have been a reason why we didn't del... haha

We previously weren't importing sys in this module, so this shouldn't be a change in behavior. sys import was added here: https://github.com/ray-project/ray/pull/27658/files#diff-f95026a08bcb464b58b036437876716d21d3b8630e61258303bcd5384d1d707cR4

rkooo567 · 2022-10-20T07:18:53Z

Also can you merge the latest master? I'd like to run many_tasks to see if the perf regression is fixed

clarkzinzow · 2022-10-20T20:29:25Z

python/ray/tests/test_runtime_env_working_dir_4.py

@@ -150,7 +150,7 @@ def check(self):
        runtime_env = {"py_modules": [S3_PACKAGE_URI]}

    # Note: We should set a bigger timeout if downloads the s3 package slowly.
-    get_timeout = 10
+    get_timeout = 2


Is this test even valid with this reduced timeout?

why do we need this change in this PR?

The test fails otherwise, where the timeout error is never raised

…orts

rkooo567

Will merge after a couple runs of many_Tasks. Please do not merge latest master @clarkzinzow. I'd like to check if this fixes the issue without @jiaodong'sPR that will be merged soon.

rkooo567 · 2022-10-21T06:34:44Z

python/ray/tests/test_runtime_env_working_dir_4.py

@@ -150,7 +150,7 @@ def check(self):
        runtime_env = {"py_modules": [S3_PACKAGE_URI]}

    # Note: We should set a bigger timeout if downloads the s3 package slowly.
-    get_timeout = 10
+    get_timeout = 2


why do we need this change in this PR?

rkooo567 · 2022-10-21T06:34:56Z

python/ray/tests/test_top_level_api.py

@@ -47,6 +54,43 @@ def test_non_ray_modules():
        assert "ray" in str(mod), f"Module {mod} should not be reachable via ray.{name}"


+def test_dynamic_subpackage_import():


love the test!

clarkzinzow · 2022-10-21T07:47:33Z

@rkooo567 sounds good, although before merging we should double-check the failing test_object_directory_failure test, as well as the tweak that I made to the runtime env test.

rkooo567 · 2022-10-21T13:24:39Z

promising sign btw. It passes the test https://buildkite.com/ray-project/release-tests-pr/builds/18731#0183f7a5-b88f-4269-b1ba-b03fdf4f9c8e. I am rerunning it

…ntime env test.

clarkzinzow · 2022-10-21T22:16:02Z

@rkooo567 No spooky CI failures in the most recent run, worker registration timeout adjustment for the runtime env test appears to have done the trick.

This is a quick and relatively safer attempt to address #29324 In #28418 we attempted to unify ray.air utils with shared utils function but triggered expensive ray.data imports. Where longer term and more robust solution should be #27658

clarkzinzow · 2022-10-24T21:10:03Z

@rkooo567 All currently failing tests were flaky in master at the time of the run, do you think this is good to merge?

clarkzinzow · 2022-10-24T22:32:17Z

Tests are looking good, merging!

…ray-project#27658)" This reverts commit 241a02e.

…#27658)" (#29659) This reverts commit 241a02e, reverting PR #27658. This PR was making some GCS tests flaky (somehow).

…dynamic. (#27658)" (#29659)" This reverts commit b0bd270.

…dynamic. (#27658)" (#29659)" (#30219) This reverts commit b0bd270.

This is a quick and relatively safer attempt to address ray-project#29324 In ray-project#28418 we attempted to unify ray.air utils with shared utils function but triggered expensive ray.data imports. Where longer term and more robust solution should be ray-project#27658 Signed-off-by: Weichen Xu <[email protected]>

…oject#27658) Certain Ray subpackages are expensive to import, either due to their size, dependencies, or their import-time logic that must be executed. E.g. a change in a Datasets import caused a regression for the many_tasks nightly test, despite many_tasks only using Ray Core. This PR delays the import of these expensive subpackages until attribute access, e.g. none of the Datasets code will be run until ray.data is accessed. Signed-off-by: Weichen Xu <[email protected]>

…ray-project#27658)" (ray-project#29659) This reverts commit 241a02e, reverting PR ray-project#27658. This PR was making some GCS tests flaky (somehow). Signed-off-by: Weichen Xu <[email protected]>

…dynamic. (ray-project#27658)" (ray-project#29659)" (ray-project#30219) This reverts commit b0bd270. Signed-off-by: Weichen Xu <[email protected]>

clarkzinzow commented Aug 8, 2022

View reviewed changes

python/ray/__init__.py Show resolved Hide resolved

clarkzinzow commented Aug 8, 2022

View reviewed changes

clarkzinzow mentioned this pull request Aug 8, 2022

[2.0][nightly-test] many-tasks regression #27606

Closed

clarkzinzow force-pushed the core/feat/dynamic-expensive-subpackage-imports branch from a0a233c to 5e8328f Compare August 8, 2022 21:50

stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Sep 8, 2022

stale bot closed this Sep 24, 2022

clarkzinzow mentioned this pull request Oct 11, 2022

[docs] Reorganize the tensor data support docs; general editing #26952

Merged

clarkzinzow reopened this Oct 18, 2022

stale bot removed the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Oct 18, 2022

clarkzinzow added the core Issues that should be addressed in Ray Core label Oct 18, 2022

clarkzinzow force-pushed the core/feat/dynamic-expensive-subpackage-imports branch 2 times, most recently from 29d05e9 to d202401 Compare October 20, 2022 02:25

Make expensive subpackage imports dynamic.

486a339

clarkzinzow force-pushed the core/feat/dynamic-expensive-subpackage-imports branch from 0a1de12 to 486a339 Compare October 20, 2022 02:51

rkooo567 self-assigned this Oct 20, 2022

rkooo567 mentioned this pull request Oct 20, 2022

[ci][release] many_tasks failed #29324

Closed

rkooo567 approved these changes Oct 20, 2022

View reviewed changes

jiaodong mentioned this pull request Oct 20, 2022

[AIR] Inline AIR level ray.data imports #29517

Merged

7 tasks

clarkzinzow commented Oct 20, 2022

View reviewed changes

clarkzinzow added 3 commits October 20, 2022 23:05

Make ray.autoscaler an eager import again.

e8a2ed1

Add test coverage.

7e55837

Merge branch 'master' into core/feat/dynamic-expensive-subpackage-imp…

5144c1b

…orts

rkooo567 reviewed Oct 21, 2022

View reviewed changes

rkooo567 added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Oct 21, 2022

Reduce worker registration timeout rather than the get timeout for ru…

b702d41

…ntime env test.

clarkzinzow merged commit 241a02e into ray-project:master Oct 24, 2022

clarkzinzow added a commit to clarkzinzow/ray that referenced this pull request Oct 25, 2022

Revert "[Prototype] [Core] Make expensive subpackage imports dynamic. (…

8e4759b

…ray-project#27658)" This reverts commit 241a02e.

clarkzinzow mentioned this pull request Oct 25, 2022

Revert "[Prototype] [Core] Make expensive subpackage imports dynamic. (#27658)" #29659

Merged

7 tasks

clarkzinzow added a commit that referenced this pull request Oct 25, 2022

Revert "[Prototype] [Core] Make expensive subpackage imports dynamic. (…

b0bd270

…#27658)" (#29659) This reverts commit 241a02e, reverting PR #27658. This PR was making some GCS tests flaky (somehow).

rickyyx added a commit that referenced this pull request Nov 11, 2022

Revert "Revert "[Prototype] [Core] Make expensive subpackage imports …

09ef244

…dynamic. (#27658)" (#29659)" This reverts commit b0bd270.

scv119 pushed a commit that referenced this pull request Nov 16, 2022

Revert "Revert "[Prototype] [Core] Make expensive subpackage imports …

3fbd9a6

…dynamic. (#27658)" (#29659)" (#30219) This reverts commit b0bd270.

clarkzinzow mentioned this pull request Feb 14, 2023

Revert "[data] Fix pandas import failures by moving it to a top-level data import" #32533

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Prototype] [Core] Make expensive subpackage imports dynamic. #27658

[Prototype] [Core] Make expensive subpackage imports dynamic. #27658

clarkzinzow commented Aug 8, 2022 •

edited

Loading

clarkzinzow Aug 8, 2022 •

edited

Loading

clarkzinzow Aug 8, 2022 •

edited

Loading

stale bot commented Sep 8, 2022

stale bot commented Sep 24, 2022

rkooo567 commented Oct 20, 2022

rkooo567 commented Oct 20, 2022

clarkzinzow commented Oct 20, 2022

rkooo567 left a comment •

edited

Loading

rkooo567 Oct 20, 2022

clarkzinzow Oct 20, 2022

rkooo567 Oct 20, 2022

clarkzinzow Oct 20, 2022

rkooo567 commented Oct 20, 2022

clarkzinzow Oct 20, 2022

rkooo567 Oct 21, 2022

clarkzinzow Oct 21, 2022

rkooo567 left a comment

rkooo567 Oct 21, 2022

rkooo567 Oct 21, 2022

clarkzinzow commented Oct 21, 2022

rkooo567 commented Oct 21, 2022

clarkzinzow commented Oct 21, 2022

clarkzinzow commented Oct 24, 2022

clarkzinzow commented Oct 24, 2022

		@@ -47,6 +54,43 @@ def test_non_ray_modules():
		assert "ray" in str(mod), f"Module {mod} should not be reachable via ray.{name}"


		def test_dynamic_subpackage_import():

[Prototype] [Core] Make expensive subpackage imports dynamic. #27658

[Prototype] [Core] Make expensive subpackage imports dynamic. #27658

Conversation

clarkzinzow commented Aug 8, 2022 • edited Loading

Related issue number

Checks

clarkzinzow Aug 8, 2022 • edited Loading

Choose a reason for hiding this comment

clarkzinzow Aug 8, 2022 • edited Loading

Choose a reason for hiding this comment

stale bot commented Sep 8, 2022

stale bot commented Sep 24, 2022

rkooo567 commented Oct 20, 2022

rkooo567 commented Oct 20, 2022

clarkzinzow commented Oct 20, 2022

rkooo567 left a comment • edited Loading

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

rkooo567 commented Oct 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkooo567 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clarkzinzow commented Oct 21, 2022

rkooo567 commented Oct 21, 2022

clarkzinzow commented Oct 21, 2022

clarkzinzow commented Oct 24, 2022

clarkzinzow commented Oct 24, 2022

clarkzinzow commented Aug 8, 2022 •

edited

Loading

clarkzinzow Aug 8, 2022 •

edited

Loading

clarkzinzow Aug 8, 2022 •

edited

Loading

rkooo567 left a comment •

edited

Loading