[DataFrame] Fixing the code formatting of the tests #2123

devin-petersohn · 2018-05-22T23:32:43Z

There were a significant number of unnecessary extra lines, formatting errors/inconsistencies, etc.

The content of the tests did not change (with the exception of quantile getting additional percentiles to test). Rather the format of the tests are now such that it the tests are in a more maintainable order.

Also for the tests only import pandas as pd to import pandas and import ray.dataframe as rdf to import ray.dataframe as pd

kunalgosar

Looks great!

pcmoritz · 2018-05-22T23:48:50Z

We should also consider turning on yapf for DataFrames (in a separate PR, see https://github.com/ray-project/ray/blob/master/.travis/yapf.sh#L16)

AmplabJenkins · 2018-05-23T00:02:02Z

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

devin-petersohn · 2018-05-23T23:39:54Z

Jenkins, retest this please.

AmplabJenkins · 2018-05-24T01:00:35Z

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

AmplabJenkins · 2018-05-24T15:50:29Z

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

AmplabJenkins · 2018-05-24T17:29:14Z

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

pschafhalter

Great work! Left comments on minor stylistic changes.

Overall, I think we have a bit of a problem in terms of stylistic consistency. For example, df can refer to Pandas or Ray dataframes. Sometimes we use pd_df (which is strange now that we import ray.dataframe as pd), other times ray_df, and sometimes expected.

Also, sometimes we use single quotes or double quotes. If not in this one, we might want to fix this in a future PR.

I also think that we might want to move all tests that accept dataframes as arguments to a specific section of the file or even another file.

pschafhalter · 2018-05-24T18:23:03Z

python/ray/dataframe/test/test_dataframe.py

 from pandas.tests.frame.common import TestData
+import ray.dataframe as pd


In many other files, we still use import pandas as pd. We'll have to update those quickly to maintain a consistent code base. Maybe create an issue?

There is an open PR: #1942

pschafhalter · 2018-05-24T18:31:24Z

python/ray/dataframe/test/test_dataframe.py

-@pytest.fixture
-def test_get_ftype_counts(ray_df, pandas_df):
-    assert(ray_df.get_ftype_counts().equals(pandas_df.get_ftype_counts()))
+    return to_pandas(ray_df1).equals(to_pandas(ray_df2))


Do we need to convert to pandas? Ray dataframes implement equals, so shouldn't ray_df1.equals(ray_df2) be equivalent and more efficient?

equals doesn't work properly, we have to use this for now.

pschafhalter · 2018-05-24T18:45:18Z

python/ray/dataframe/test/test_dataframe.py

-                              'col4': [12, 13, 14, 15],
-                              'col5': [0, 0, 0, 0]})
-    ray_df = from_pandas(pandas_df, 2)
+    pandas_df = pandas.DataFrame({'col1': [0, 1, 2, 3],


This seems like it could be refactored to use something like create_test_dataframe()

pschafhalter · 2018-05-24T18:46:12Z

python/ray/dataframe/test/test_dataframe.py

-                              'col3': [8.0, 9.0, 10.0, 11.0],
-                              'col4': [12.0, 13.0, 14.0, 15.0],
-                              'col5': [0.0, 0.0, 0.0, 0.0]})
+    pandas_df = pandas.DataFrame({'col1': [0.0, 1.0, 2.0, 3.0],


This too, but cast to float.

As part of the robustness effort, it's probably not a good idea to use the same dataframe across all the tests. Ideally, the test dataframes will be changed.

pschafhalter · 2018-05-24T18:48:11Z

python/ray/dataframe/test/test_dataframe.py

-                       'col5': [0, 0, 0, 0]})
-
-    return from_pandas(df, 2)
+    return pd.DataFrame({'col1': [0, 1, 2, 3],


This creates an int dataframe. It's possible that tests which use this function won't catch bugs for float, datetime, and other objects.

At some point we need to do a rewrite of the tests, this is a known issue

I don't see an issue for this. Would you like to make one?

Anyway, it doesn't hurt to make the current tests more robust.

We need a good design doc on testing as well (robust and future proof, should be self-contained but does not need to be as exhaustive as pandas test, a good pytest structure that enable tests to run in parallel...). Can you draft one? In the meantime, I don't think we should refactor the tests. This is just a code formatting PR after all.

Made the issue at: #2134

pschafhalter · 2018-05-24T19:11:07Z

python/ray/dataframe/test/test_dataframe.py

    assert ray_df.memory_usage(index=True).at['Index'] is not None
    assert ray_df.memory_usage(deep=True).sum() >= \
        ray_df.memory_usage(deep=False).sum()


 def test_merge():
-    ray_df = rdf.DataFrame({"col1": [0, 1, 2, 3], "col2": [4, 5, 6, 7],
-                            "col3": [8, 9, 0, 1], "col4": [2, 4, 5, 6]})
+    ray_df = pd.DataFrame({"col1": [0, 1, 2, 3], "col2": [4, 5, 6, 7],


pschafhalter · 2018-05-24T19:11:13Z

python/ray/dataframe/test/test_dataframe.py


-    ray_df2 = rdf.DataFrame({"col1": [0, 1, 2], "col2": [1, 5, 6]})
+    ray_df2 = pd.DataFrame({"col1": [0, 1, 2], "col2": [1, 5, 6]})


pschafhalter · 2018-05-24T19:12:04Z

python/ray/dataframe/test/test_dataframe.py

-                        [1.5, np.nan, 3.],
-                        [1.5, np.nan, 3],
-                        [1.5, np.nan, 3]])
+    df = pd.DataFrame([[1.5, np.nan, 3.],


I don't think changing this one is necessary. It's used once.

pschafhalter · 2018-05-24T19:12:23Z

python/ray/dataframe/test/test_dataframe.py

-    pd_df = pd.DataFrame({'year': [2015, 2016],
-                          'month': [2, 3],
-                          'day': [4, 5]})
+    ray_df = pd.DataFrame({'year': [2015, 2016],


pschafhalter · 2018-05-24T19:12:29Z

python/ray/dataframe/test/test_dataframe.py

-                          'C': [1, 2, 3]})
-
-    assert ray_df_equals_pandas(rdf.get_dummies(ray_df), pd.get_dummies(pd_df))
+    ray_df = pd.DataFrame({'A': ['a', 'b', 'a'],


kunalgosar · 2018-05-24T21:10:24Z

I think it makes sense to fix the issues that this PR directly touches, given this is just a formatting fix, this probably isn't the right place to try to do a full refactor of the test code. I particularly disagree with the comments on code that this PR does not touch, they should be fixed at some point. But this is not the right place.

I'll create an issue for tracking purposes. But, at some point we should refactor all of the test code together to make it collectively more robust.

AmplabJenkins · 2018-05-25T00:43:42Z

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

AmplabJenkins · 2018-05-25T01:19:24Z

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

pschafhalter · 2018-05-26T18:24:22Z

Merged. Thanks @devin-petersohn!

* master: [autoscaler] GCP node provider (ray-project#2061) [xray] Evict tasks from the lineage cache (ray-project#2152) [ASV] Add ray.init and simple Ray benchmarks (ray-project#2166) Re-encrypt key for uploading to S3 from travis to use travis-ci.com. (ray-project#2169) [rllib] Fix A3C PyTorch implementation (ray-project#2036) [JavaWorker] Do not kill local-scheduler-forked workers in RunManager.cleanup (ray-project#2151) Update Travis CI badge from travis-ci.org to travis-ci.com. (ray-project#2155) Implement Python global state API for xray. (ray-project#2125) [xray] Improve flush algorithm for the lineage cache (ray-project#2130) Fix support for actor classmethods (ray-project#2146) Add empty df test (ray-project#1879) [JavaWorker] Enable java worker support (ray-project#2094) [DataFrame] Fixing the code formatting of the tests (ray-project#2123) Update resource documentation (remove outdated limitations). (ray-project#2022) bugfix: use array redis_primary_addr out of its scope (ray-project#2139) Fix infinite retry in Push function. (ray-project#2133) [JavaWorker] Changes to the directory under src for support java worker (ray-project#2093) Integrate credis with Ray & route task table entries into credis. (ray-project#1841)

kunalgosar approved these changes May 22, 2018

View reviewed changes

devin-petersohn force-pushed the df_fix_test_formatting branch from aeffe55 to 9b022cc Compare May 24, 2018 14:40

pschafhalter suggested changes May 24, 2018

View reviewed changes

devin-petersohn added 4 commits May 24, 2018 16:20

Fixing the code formatting of the tests

d1f189f

Fixing tests and removing from_pandas

6378a30

Addressing comment

60a4ca8

Addressing comments

de8f8d7

devin-petersohn force-pushed the df_fix_test_formatting branch from 7fefadd to de8f8d7 Compare May 24, 2018 23:38

Fix lint

83193a1

pschafhalter approved these changes May 26, 2018

View reviewed changes

pschafhalter merged commit 74cca3b into ray-project:master May 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DataFrame] Fixing the code formatting of the tests #2123

[DataFrame] Fixing the code formatting of the tests #2123

devin-petersohn commented May 22, 2018

kunalgosar left a comment

pcmoritz commented May 22, 2018

AmplabJenkins commented May 23, 2018

devin-petersohn commented May 23, 2018

AmplabJenkins commented May 24, 2018

AmplabJenkins commented May 24, 2018

AmplabJenkins commented May 24, 2018

pschafhalter left a comment

pschafhalter May 24, 2018

kunalgosar May 24, 2018

pschafhalter May 24, 2018

devin-petersohn May 24, 2018

pschafhalter May 24, 2018

pschafhalter May 24, 2018

kunalgosar May 24, 2018

pschafhalter May 24, 2018

kunalgosar May 24, 2018

pschafhalter May 24, 2018

simon-mo May 24, 2018

kunalgosar May 24, 2018

pschafhalter May 24, 2018

pschafhalter May 24, 2018

pschafhalter May 24, 2018

devin-petersohn May 25, 2018

pschafhalter May 24, 2018

pschafhalter May 24, 2018

kunalgosar commented May 24, 2018

AmplabJenkins commented May 25, 2018

AmplabJenkins commented May 25, 2018

pschafhalter commented May 26, 2018

		from pandas.tests.frame.common import TestData
		import ray.dataframe as pd


		ray_df2 = rdf.DataFrame({"col1": [0, 1, 2], "col2": [1, 5, 6]})
		ray_df2 = pd.DataFrame({"col1": [0, 1, 2], "col2": [1, 5, 6]})

[DataFrame] Fixing the code formatting of the tests #2123

[DataFrame] Fixing the code formatting of the tests #2123

Conversation

devin-petersohn commented May 22, 2018

kunalgosar left a comment

Choose a reason for hiding this comment

pcmoritz commented May 22, 2018

AmplabJenkins commented May 23, 2018

devin-petersohn commented May 23, 2018

AmplabJenkins commented May 24, 2018

AmplabJenkins commented May 24, 2018

AmplabJenkins commented May 24, 2018

pschafhalter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kunalgosar commented May 24, 2018

AmplabJenkins commented May 25, 2018

AmplabJenkins commented May 25, 2018

pschafhalter commented May 26, 2018