TEST-#3085: Add testing for Ray Client #3140
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Some alternate ways these tests can be added in are:
|
|
Codecov Report
@@ Coverage Diff @@
## master #3140 +/- ##
===========================================
- Coverage 82.91% 66.03% -16.89%
===========================================
Files 146 146
Lines 14993 15010 +17
===========================================
- Hits 12432 9912 -2520
- Misses 2561 5098 +2537
Continue to review full report at Codecov.
|
|
|
|
@devin-petersohn , what do you think is a reasonable ETA for getting this merged? |
|
@ckw017 Can you rebase this on current master? Ray 1.4 support is merged. |
Signed-off-by: Chris Wong <chriskw.xyz@gmail.com>
Signed-off-by: Chris Wong <chriskw.xyz@gmail.com>
Signed-off-by: Chris Wong <chriskw.xyz@gmail.com>
|
@devin-petersohn Rebased with master and uncommented the tests that were formerly broken on before the 0.10 updates. LMK if you want anything added or changed! |
vnlitvinov
reviewed
Jun 10, 2021
modin/conftest.py
Outdated
| import subprocess | ||
|
|
||
| port = "50051" | ||
| # Clean up any extra processes from previous runs |
There was a problem hiding this comment.
I would say it's a bad idea, who knows which processes are running there from some parallel session (potentially by some other user)?
There was a problem hiding this comment.
Yeah that's reasonable, I'll get rid of those.
Signed-off-by: Chris Wong <chriskw.xyz@gmail.com>
The main issues we want to catch were things like #2688, which could be caught pretty quickly with just a few tests. Rather than running through all the tests covered in test-all we can switch to just a subset and only run on one version of Python, if you want to cut down on the clutter in the CI. |
There was a problem hiding this comment.
@vnlitvinov I am okay with adding these on a per-commit basis. The tests don't take that long.
@ckw017 Left a comment, thanks!
| - run: pip install -r requirements-dev.txt | ||
| # Use a ray master commit that includes the fix here: https://github.com/ray-project/ray/pull/16278 | ||
| # Can be changed after a Ray version > 1.4 is released. | ||
| - run: pip install https://s3-us-west-2.amazonaws.com/ray-wheels/master/c8e3ed9eec30119092ef966ee7b8982c8954c333/ray-2.0.0.dev0-cp37-cp37m-manylinux2014_x86_64.whl |
There was a problem hiding this comment.
Can this (and below) be removed? Or do we need to keep testing on this commit specifically? I think we want Ray 1.4 here.
There was a problem hiding this comment.
We'll probably want to keep that in for now, since it prevents the: pandas has no attribute 'compat' issue altogether, which seemed to crop up frequently on Github Actions machines when I was testing this PR out. We can push for that to be in 1.4.1 down the line so that we don't have to keep testing against a specific commit.
| @@ -239,6 +239,16 @@ class TestDatasetSize(EnvironmentVariable, type=str): | |||
| choices = ("Small", "Normal", "Big") | |||
|
|
|||
|
|
|||
| class TestRayClient(EnvironmentVariable, type=bool): | |||
There was a problem hiding this comment.
As a side note here, we should probably have configs specific for tests elsewhere (not needed to change here).
7 tasks
|
@devin-petersohn @vnlitvinov I've opened #3147 which has the same changes as this PR, but only runs on push-to-master. Feel free to merge in whichever one works better on your end, and close the other. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Chris Wong chriskw.xyz@gmail.com
What do these changes do?
On code side there's a new environment variable that can be set to start a ray client connection before the test session starts.
For push/ci, currently installing a wheel slightly ahead of Ray 1.4 to fix the import deserialization race condition, which seemed to crop up extra frequently when using ray client. Since client is a bit finicky with multiple connections at the moment the
-n 2flag with pytest doesn't work, so I separated out the tests in the matrix so that they wouldn't bottleneck the completion time (similar to what's being done for test windows). I've also left out the following tests which are failing due to reasons unrelated to ray client (primarily seems like there's been a change with how errors are propagated since 1.1 which the test suite doesn't yet handle)modin/pandas/test/dataframe/test_binary.pymodin/pandas/test/dataframe/test_map_metadata.pymodin/pandas/test/dataframe/test_reduction.pymodin/pandas/test/dataframe/test_udf.pymodin/pandas/test/test_series.pymodin/experimental/pandas/test/test_io_exp.pyflake8 modinblack --check modingit commit -sdocs/developer/architecture.rstis up-to-date