Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Ray] Add unit tests for ray client + modin #3085

Closed
ckw017 opened this issue May 24, 2021 · 7 comments · Fixed by #3147
Closed

[Ray] Add unit tests for ray client + modin #3085

ckw017 opened this issue May 24, 2021 · 7 comments · Fixed by #3147
Labels
new feature/request 💬 Requests and pull requests for new features

Comments

@ckw017
Copy link
Contributor

ckw017 commented May 24, 2021

Is your feature request related to a problem? Please describe.
A clear and concise description of what the problem is. What kind of performance improvements would you like to see with this new API?

Wanted to open discussion about adding unit tests using ray client to this repo's CI. If there are any examples that should be covered that would be handy to know, as well as any guidelines for where to put these tests. Example issue with ray-client on the ray repo ray-project/ray#14857.

@amogkam @devin-petersohn

@ckw017 ckw017 added the new feature/request 💬 Requests and pull requests for new features label May 24, 2021
@amogkam
Copy link

amogkam commented May 24, 2021

Having some CI coverage here would really help with making sure we have native support for Modin with Ray Client. We can also add tests for this on the Ray CI.

Another relevant issue is here #2688

@devin-petersohn
Copy link
Collaborator

Thanks @ckw017 and @amogkam. I agree it would be good if we could add tests for basic functionality.

@amogkam
Copy link

amogkam commented May 25, 2021

@devin-petersohn any guidance on how to go about adding this to modin CI? I think ideally we can just run the same tests that currently exist for modin on ray, except with ray client.

@devin-petersohn
Copy link
Collaborator

For this, we would probably want to add a separate job within the existing CI and push for the GitHub Actions (not necessary for our TeamCity tests).

We would need to start the client before running the tests. Modin will automatically connect to the client if you run with environment variable MODIN_RAY_CLUSTER. Here's our current github actions workflow: https://github.com/modin-project/modin/blob/master/.github/workflows/ci.yml

@ckw017
Copy link
Contributor Author

ckw017 commented Jun 7, 2021

@devin-petersohn Hey Devin, just wanted to check two things that would help get this closed:

  1. It looks like to add tests for modin onto Ray's CI, we need a version of Modin with the fix in FIX-#2844: Check for both types of ray.ObjectRef #2851. It looks like this was a bit after 0.9.1 was released, so wanted to check if it was possible to get 0.9.2 out so that we can run our tests against that.
  2. For getting tests for Ray Client onto Modin's CI, we would need to use a nightly ray wheel since [core] Use function_actor_manager.lock when deserializing ray-project/ray#16278 won't be part of Ray 1.4, and is needed to get the tests to run consistently. It looks like all of the current unit tests run against Ray <= 1.1.0, so I wanted to check if you currently test against nightly anywhere so that we can stick the client tests there. Alternatively, we can add the client tests onto your CI as planned (as a separate job) and install a wheel including the patch just for the client tests

@ckw017
Copy link
Contributor Author

ckw017 commented Jun 7, 2021

^ First thing should be fine for now, we'll just install the latest commit when testing for now.

@devin-petersohn
Copy link
Collaborator

@ckw017 thanks for following up!

1.) We are planning to release 0.10 tomorrow (probably 😄). After release it'll probably be good to test the stable release since we are planning major changes for the release after 0.10.
2.) Ray 1.4 will work with Modin given recent patches (e.g., ray-project/ray#15990). We also have a nightly test that runs on the nightly docker container, but that probably needs to be changed to pip install ray && ray install-nightly because it's been failing frequently (see here: https://github.com/modin-project/modin/actions/workflows/push-to-master.yml). Ideally we have Ray 1.4 in 0.10. I will try to make that happen.

ckw017 added a commit to ckw017/modin that referenced this issue Jun 10, 2021
Signed-off-by: Chris Wong <chriskw.xyz@gmail.com>
ckw017 added a commit to ckw017/modin that referenced this issue Jun 10, 2021
Signed-off-by: Chris Wong <chriskw.xyz@gmail.com>
ckw017 added a commit to ckw017/modin that referenced this issue Jun 10, 2021
Signed-off-by: Chris Wong <chriskw.xyz@gmail.com>
ckw017 added a commit to ckw017/modin that referenced this issue Jun 10, 2021
Signed-off-by: Chris Wong <chriskw.xyz@gmail.com>
ckw017 added a commit to ckw017/modin that referenced this issue Jun 14, 2021
….yml

Signed-off-by: Chris Wong <chriskw.xyz@gmail.com>
devin-petersohn pushed a commit that referenced this issue Jun 15, 2021
Signed-off-by: Chris Wong <chriskw.xyz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature/request 💬 Requests and pull requests for new features
Projects
None yet
3 participants