Skip to content

[ray_client] Support calling functions from other functions and correct the tests#12141

Merged
ericl merged 4 commits intoray-project:masterfrom
barakmich:client_f_g
Nov 25, 2020
Merged

[ray_client] Support calling functions from other functions and correct the tests#12141
ericl merged 4 commits intoray-project:masterfrom
barakmich:client_f_g

Conversation

@barakmich
Copy link
Copy Markdown
Contributor

@barakmich barakmich commented Nov 18, 2020 •
edited
Loading

Why are these changes needed?

Related issue number

Closes #11975

Checks

  • 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 :(

ericl
ericl reviewed Nov 19, 2020
View reviewed changes
python/ray/experimental/client/server/core_ray_api.py Outdated
Copy link
Copy Markdown
Contributor

@ericl ericl Nov 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to apply the remote decorator args here (e.g., num_cpus=1)?

Copy link
Copy Markdown
Contributor Author

@barakmich barakmich Nov 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remote decorator args are probably a followup, but yeah, this is where it gets piped in.

Copy link
Copy Markdown
Contributor

@ericl ericl Nov 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add this? Should be simple enough.

Copy link
Copy Markdown
Contributor Author

@barakmich barakmich Nov 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They require another field in the proto -- they tell us how to execute the function, instead of the parameters to execute that function.

Again, it's a simple addition, but orthogonal to the change.

Copy link
Copy Markdown
Contributor

@ericl ericl Nov 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Can you follow-up?

Copy link
Copy Markdown
Contributor Author

@barakmich barakmich Nov 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#12282 for tracking

ericl
ericl reviewed Nov 19, 2020
View reviewed changes
python/ray/experimental/client/common.py Outdated
Copy link
Copy Markdown
Contributor

@ericl ericl Nov 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm why not make this a member of this class instead of ray?

Copy link
Copy Markdown
Contributor Author

@barakmich barakmich Nov 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this class can't contain the API (it needs the ray trampoline depending on which end is calling this method)

ericl
ericl reviewed Nov 19, 2020
View reviewed changes
python/ray/experimental/client/common.py Outdated
Copy link
Copy Markdown
Contributor

@ericl ericl Nov 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need set_remote_func / run_remote_func now?

Copy link
Copy Markdown
Contributor Author

@barakmich barakmich Nov 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair cop. Might as well just use it.

@ericl ericl added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Nov 19, 2020
@ericl ericl self-assigned this Nov 19, 2020
@ericl
Copy link
Copy Markdown
Contributor

ericl commented Nov 19, 2020

Needs rebasing...

@barakmich barakmich added tests-ok The tagger certifies test failures are unrelated and assumes personal liability. and removed @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. labels Nov 23, 2020
@barakmich barakmich added this to the Ray Client milestone Nov 23, 2020
@ericl
Copy link
Copy Markdown
Contributor

ericl commented Nov 23, 2020

//python/ray/tests:test_experimental_client TIMEOUT in 3 out of 3 in 75.0s
Stats over 3 runs: max = 75.0s, min = 75.0s, avg = 75.0s, dev = 0.0s
/home/travis/.cache/bazel/_bazel_travis/b88c129a127452fc94033a29d9f90e20/execroot/com_github_ray_project_ray/bazel-out/k8-opt/testlogs/python/ray/tests/test_experimental_client/test.log
/home/travis/.cache/bazel/_bazel_travis/b88c129a127452fc94033a29d9f90e20/execroot/com_github_ray_project_ray/bazel-out/k8-opt/testlogs/python/ray/tests/test_experimental_client/test_attempts/attempt_1.log
/home/travis/.cache/bazel/_bazel_travis/b88c129a127452fc94033a29d9f90e20/execroot/com_github_ray_project_ray/bazel-out/k8-opt/testlogs/python/ray/tests/test_experimental_client/test_attempts/attempt_2.log

@ericl ericl added @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. and removed tests-ok The tagger certifies test failures are unrelated and assumes personal liability. labels Nov 23, 2020
@ericl
Copy link
Copy Markdown
Contributor

ericl commented Nov 23, 2020

@barakmich looks like tests are hanging

@barakmich
Copy link
Copy Markdown
Contributor Author

barakmich commented Nov 25, 2020

Failures are now outside the client

@barakmich barakmich mentioned this pull request Nov 25, 2020
Merged
6 tasks
@ericl ericl merged commit 4066056 into ray-project:master Nov 25, 2020
amogkam added a commit to amogkam/ray that referenced this pull request Nov 26, 2020
@amogkam
Revert "[ray_client] Support calling functions from other functions a…
bb27b87 
…nd correct the tests (ray-project#12141)"

This reverts commit 4066056.
amogkam added a commit to amogkam/ray that referenced this pull request Nov 26, 2020
@amogkam
Revert "Revert "[ray_client] Support calling functions from other fun…
730389f 
…ctions and correct the tests (ray-project#12141)""

This reverts commit bb27b87.
simon-mo pushed a commit that referenced this pull request Nov 26, 2020
@amogkam
[CI] Fix windows build (#12415)
e0573df 
* attempt to fix windows

* fix syntax

* try again

* try again

* try again

* Revert "[ray_client] Support calling functions from other functions and correct the tests (#12141)"

This reverts commit 4066056.

* Revert

* Revert "Revert "[ray_client] Support calling functions from other functions and correct the tests (#12141)""

This reverts commit bb27b87.

* please work

* works

* fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericl ericl ericl approved these changes

Assignees

@ericl ericl

Labels

@author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer.

Projects

None yet

Milestone

Ray Client

Development

Successfully merging this pull request may close these issues.

Support calling remote functions from remote functions (f->g)

2 participants

@barakmich @ericl