Skip to content

error message on non-integral args with ray.remote#10221

Merged
edoakes merged 3 commits intoray-project:masterfrom
kishansagathiya:issue_8894
Aug 28, 2020
Merged

error message on non-integral args with ray.remote#10221
edoakes merged 3 commits intoray-project:masterfrom
kishansagathiya:issue_8894

Conversation

@kishansagathiya
Copy link
Contributor

@kishansagathiya kishansagathiya commented Aug 20, 2020 •
edited
Loading

This PR type checks arguments for ray.remote and checks if they
belong to the valid range for that argument and shows informative error
if otherwise.

Why are these changes needed?

Related issue number

Fixes #8894

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/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)

@kishansagathiya
error message on non-integral args with ray.remote
5680cac 
This commit type checks arguments for ray.remote and checks if they
belong to the valid range for that argument and shows informative error
if otherwise.

Fixes #8894
@kishansagathiya kishansagathiya marked this pull request as ready for review August 22, 2020 13:03
@kishansagathiya
Copy link
Contributor Author

kishansagathiya commented Aug 22, 2020

Hi, this is ready for review. As far as I understand, failing tests here are unrelated to the PR.

There are tests where value of num_cpus is float. Can you tell me what it means to have say 0.1 as num_cpus?

edoakes
edoakes reviewed Aug 25, 2020
View reviewed changes
Copy link
Collaborator

@edoakes edoakes left a comment

Choose a reason for hiding this comment

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

This looks great @kishansagathiya! Would you mind also adding some basic tests for these?

@edoakes
Copy link
Collaborator

edoakes commented Aug 25, 2020

Also @kishansagathiya, yes floats are valid for num_cpus. We don't actually pin CPUs and users may want more than one task per CPU (e.g., if they're IO intensive)

@edoakes edoakes added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Aug 25, 2020
@kishansagathiya
Copy link
Contributor Author

kishansagathiya commented Aug 25, 2020

@edoakes

This looks great @kishansagathiya! Would you mind also adding some basic tests for these?

Sure

@kishansagathiya
Added tests for invalid args for ray.remote
013d4ba 
This commit includes tests to check
	- when ray.remote is passed with an inappropriate arguments, it
throughs ValueError exceptions

issue #8894
@kishansagathiya
Copy link
Contributor Author

kishansagathiya commented Aug 27, 2020 •
edited
Loading

@edoakes
Added tests only check if things fail on invalid arguments.

It doesn't check that things work on valid arguments like positive integers and -1 in some cases. Let me know if those tests needs to be added.

edoakes
edoakes approved these changes Aug 27, 2020
View reviewed changes
Copy link
Collaborator

@edoakes edoakes left a comment

Choose a reason for hiding this comment

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

LGTM

@rkooo567
Copy link
Contributor

rkooo567 commented Aug 28, 2020

@kishansagathiya Can you merge the latest master? There were some master failures, and it should've been fixed by now.

@edoakes edoakes merged commit 2afb54c into ray-project:master Aug 28, 2020
@edoakes
Copy link
Collaborator

edoakes commented Aug 28, 2020

@kishansagathiya merged it, thanks for your contribution!

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

Reviewers

@edoakes edoakes edoakes approved these changes

Assignees

@edoakes edoakes

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

No milestone

Development

Successfully merging this pull request may close these issues.

No proper error message when num_return_vals is negative or not integral

3 participants

@kishansagathiya @edoakes @rkooo567