Deprecate _remote function #7676

allenanswerzq · 2020-03-20T11:45:52Z

Why are these changes needed?

Deprecate _remote function.

Related issue number

Closes #7040

Checks

I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://ray.readthedocs.io/en/latest/.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failure rates at https://ray-travis-tracker.herokuapp.com/.
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested (please justify below)

AmplabJenkins · 2020-03-20T11:48:42Z

Can one of the admins verify this patch?

allenanswerzq · 2020-03-20T12:31:06Z

I think also need to change all tests that are currently use _remote function.

AmplabJenkins · 2020-03-20T12:49:00Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/23432/
Test FAILed.

AmplabJenkins · 2020-03-20T14:28:07Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/23438/
Test FAILed.

python/ray/util/__init__.py

AmplabJenkins · 2020-03-20T16:39:24Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/23440/
Test FAILed.

AmplabJenkins · 2020-03-20T17:08:33Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/23442/
Test FAILed.

AmplabJenkins · 2020-03-21T03:50:41Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/23476/
Test FAILed.

AmplabJenkins · 2020-03-21T04:39:34Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/23478/
Test FAILed.

AmplabJenkins · 2020-03-21T04:54:41Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/23481/
Test FAILed.

AmplabJenkins · 2020-03-21T04:57:30Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/23479/
Test FAILed.

AmplabJenkins · 2020-03-21T05:44:14Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/23482/
Test FAILed.

AmplabJenkins · 2020-03-21T06:36:22Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/23484/
Test FAILed.

AmplabJenkins · 2020-03-21T09:50:50Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/23487/
Test PASSed.

edoakes

Thanks for doing this! Left a few comments to clean things up some more.

ci/long_running_tests/workloads/many_drivers.py

python/ray/actor.py

edoakes · 2020-03-24T20:56:18Z

python/ray/remote_function.py

Why do we need this internal_called flag? Can we just remove all instances of calling this instead?

Currently, there are mainly two places will call this _remote method, one place is inside the class definition which is good and intended, the other place is outside of the class, we only want to raise a warning message when the latter case happens, thus need this flag to distinguish.

I think we can just get rid of _remote() entirely instead. Looks like .remote() just calls ._remote() directly - we can just move the implementation to .remote() and leave a deprecation error in ._remote()

python/ray/util/deprecation.py

AmplabJenkins · 2020-03-26T06:35:04Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/23728/
Test PASSed.

edoakes

I think the docstrings for .options() for both actor classes and remote functions need to be updated to remove the reference to ._remote() as well.

ci/long_running_tests/workloads/many_tasks.py

ci/performance_tests/test_performance.py

edoakes · 2020-03-26T15:28:32Z

python/ray/remote_function.py

I think we can just get rid of _remote() entirely instead. Looks like .remote() just calls ._remote() directly - we can just move the implementation to .remote() and leave a deprecation error in ._remote()

edoakes · 2020-03-26T15:29:19Z

python/ray/tests/test_basic.py

+    ray.get(a2.method.remote())

-    id1, id2, id3, id4 = a.method._remote(
+    id1, id2, id3, id4 = a.method.remote(


need to change this to use .options().remote()

Here a is Actor and the ActorMethod class which a.method is doesn't even have the .options function.

allenanswerzq · 2020-03-26T16:48:03Z

python/ray/remote_function.py

-            return self._remote(args=args, kwargs=kwargs)
+            return self._remote(args=args, kwargs=kwargs, internal_called=True)

        self.remote = _remote_proxy


@edoakes If we remove _remote completely, then this wrapper here that used to keep function's signature also needs to be removed. for detail see #4985. I tried and failed to find a better way to remove the _remote method and also keep the function signature at the same time. Note that we must keep the function signature otherwise it will raise an exception at this place:

ray/python/ray/signature.py

Lines 112 to 118 in 6ce8b63

reconstructed_signature = inspect.Signature(

parameters=signature_parameters)

try:

reconstructed_signature.bind(*args, **kwargs)

except TypeError as exc:

raise TypeError(str(exc))

list_args = []

allenanswerzq changed the title ~~Deprecate _remote function~~ [WIP] Deprecate _remote function Mar 20, 2020

allenanswerzq force-pushed the add-deprecation branch 2 times, most recently from 17117be to 9bf28ee Compare March 20, 2020 16:12

allenanswerzq commented Mar 20, 2020

View reviewed changes

python/ray/util/__init__.py Outdated Show resolved Hide resolved

allenanswerzq force-pushed the add-deprecation branch 5 times, most recently from 4459fa8 to 88106dd Compare March 21, 2020 03:50

allenanswerzq force-pushed the add-deprecation branch from 88106dd to 57df465 Compare March 21, 2020 04:01

allenanswerzq force-pushed the add-deprecation branch from 57df465 to ed457e7 Compare March 21, 2020 04:47

allenanswerzq force-pushed the add-deprecation branch from ed457e7 to 2804251 Compare March 21, 2020 05:33

allenanswerzq force-pushed the add-deprecation branch from 2804251 to 678b058 Compare March 21, 2020 08:36

allenanswerzq changed the title ~~[WIP] Deprecate _remote function~~ Deprecate _remote function Mar 21, 2020

simon-mo assigned edoakes Mar 24, 2020

simon-mo requested a review from edoakes March 24, 2020 20:33

edoakes reviewed Mar 24, 2020

View reviewed changes

allenanswerzq added 2 commits March 26, 2020 12:50

Add deprecation warning to _remote method

cf9619c

Replace all _remote calls into .options calls

f7eb785

allenanswerzq force-pushed the add-deprecation branch from 678b058 to f7eb785 Compare March 26, 2020 05:10

edoakes reviewed Mar 26, 2020

View reviewed changes

allenanswerzq commented Mar 26, 2020

View reviewed changes

allenanswerzq mentioned this pull request Mar 29, 2020

Deprecate ._remote() and replace all instances with .options() for remote functions and actors. #7040

Closed

allenanswerzq closed this Jun 8, 2020

allenanswerzq deleted the add-deprecation branch June 8, 2020 09:38

allenanswerzq restored the add-deprecation branch July 16, 2020 02:32

allenanswerzq deleted the add-deprecation branch August 19, 2020 04:35

	reconstructed_signature = inspect.Signature(
	parameters=signature_parameters)
	try:
	reconstructed_signature.bind(args, *kwargs)
	except TypeError as exc:
	raise TypeError(str(exc))
	list_args = []

Deprecate _remote function #7676

Deprecate _remote function #7676

Uh oh!

Conversation

allenanswerzq commented Mar 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are these changes needed?

Related issue number

Checks

Uh oh!

AmplabJenkins commented Mar 20, 2020

Uh oh!

allenanswerzq commented Mar 20, 2020

Uh oh!

AmplabJenkins commented Mar 20, 2020

Uh oh!

AmplabJenkins commented Mar 20, 2020

Uh oh!

Uh oh!

AmplabJenkins commented Mar 20, 2020

Uh oh!

AmplabJenkins commented Mar 20, 2020

Uh oh!

AmplabJenkins commented Mar 21, 2020

Uh oh!

AmplabJenkins commented Mar 21, 2020

Uh oh!

AmplabJenkins commented Mar 21, 2020

Uh oh!

AmplabJenkins commented Mar 21, 2020

Uh oh!

AmplabJenkins commented Mar 21, 2020

Uh oh!

AmplabJenkins commented Mar 21, 2020

Uh oh!

AmplabJenkins commented Mar 21, 2020

Uh oh!

edoakes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

edoakes Mar 24, 2020

Choose a reason for hiding this comment

Uh oh!

allenanswerzq Mar 25, 2020

Choose a reason for hiding this comment

Uh oh!

edoakes Mar 26, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AmplabJenkins commented Mar 26, 2020

Uh oh!

edoakes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

edoakes Mar 26, 2020

Choose a reason for hiding this comment

Uh oh!

edoakes Mar 26, 2020

Choose a reason for hiding this comment

Uh oh!

allenanswerzq Mar 26, 2020

Choose a reason for hiding this comment

Uh oh!

allenanswerzq Mar 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

allenanswerzq commented Mar 20, 2020 •

edited

Loading

allenanswerzq Mar 26, 2020 •

edited

Loading