Add ray tests to ci configuration against the development version of cloudpickle #256

williamma12 · 2019-03-11T23:02:14Z

Similar to #236, this pr adds the ability to test the development version cloudpickle against ray.

Fixes #238

codecov · 2019-03-11T23:08:45Z

Codecov Report

Merging #256 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #256   +/-   ##
=======================================
  Coverage   88.05%   88.05%           
=======================================
  Files           1        1           
  Lines         544      544           
  Branches      107      107           
=======================================
  Hits          479      479           
  Misses         42       42           
  Partials       23       23

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 936f16f...5edfb20. Read the comment docs.

codecov · 2019-03-11T23:08:45Z

Codecov Report

Merging #256 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #256   +/-   ##
=======================================
  Coverage   88.05%   88.05%           
=======================================
  Files           1        1           
  Lines         544      544           
  Branches      107      107           
=======================================
  Hits          479      479           
  Misses         42       42           
  Partials       23       23

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 936f16f...78ea88b. Read the comment docs.

robertnishihara

Let's start with just one or two really simple tests, e.g., tests/test_mini.py or something like that. Currently this adds too much time to the CI.

Also, I'm not sure if any code in this file is actually shared with the way we do tests for the other thirdparty projects, so I think it may be a lot cleaner to put all of the code in the section for the Ray travis job instead of factoring it out. Do you know what I mean?

    - os: linux
      env: PROJECT=ray TEST_REQUIREMENTS="setproctitle hypothesis psutil flaky networkx tensorflow scipy Cython==0.29"
           PROJECT_URL=https://github.com/ray-project/ray.git
PYTEST_ARGS="--duration=10"

robertnishihara · 2019-03-11T23:55:24Z

.travis.yml

+            pushd ..;
+            git clone $PROJECT_URL;
+            pushd ray/examples/cython;
+            python setup.py install;


let's avoid building the cython tests for now

.travis.yml

williamma12 · 2019-03-12T06:02:38Z

Creating separate install and script sections increased readability but did result in some duplicated code. Let me know which is the preferred version! I personally prefer this one with a bit of code duplication for the sake of readability.

Also, I am open to adding more tests than just test_basic.py and test_mini.py @robertnishihara

.travis.yml

robertnishihara · 2019-03-13T04:59:53Z

.travis.yml

      if: commit_message =~ /(\[ci downstream\]|\[ci joblib\])/
+    - os: linux
+      env: PROJECT=ray TEST_REQUIREMENTS="setproctitle hypothesis psutil"
+           PROJECT_URL=https://github.com/ray-project/ray.git


We can remove PROJECT_URL, right?

.travis.yml

robertnishihara · 2019-03-13T05:00:53Z

.travis.yml

+      install:
+        - pip install .
+        - pip install --upgrade -r dev-requirements.txt
+        - pip install tornado


What istornado for?

I'm not sure, I was part of the install not specific to any third-party project so I'm assuming it's something that cloudpickle needs for its tests?

cloudpickle/.travis.yml

Lines 86 to 93 in b192238

install:

- pip install .

- pip install --upgrade -r dev-requirements.txt

- pip install tornado

- if [[ $TRAVIS_PYTHON_VERSION != 'pypy'* ]]; then

pip install numpy scipy;

fi

- if [[ $PROJECT != "" ]]; then

EDIT: I tried it without the lines in question and it worked

robertnishihara · 2019-03-13T05:01:23Z

.travis.yml

+        - pip install .
+        - pip install --upgrade -r dev-requirements.txt
+        - pip install tornado
+        - if [[ $TRAVIS_PYTHON_VERSION != 'pypy'* ]]; then


I don't think this if statement is necessary.

We also don't need scipy

see above comment for tornado

.travis.yml

robertnishihara · 2019-03-14T06:24:07Z

@williamma12 it's starting to look pretty good. By the way, can you test that introducing a bug in cloudpickle caueses this test to fail?

williamma12 · 2019-03-16T02:51:11Z

@robertnishihara I tested it by adding the following error to cloudpickle and the tests for ray failed

cloudpickle/cloudpickle/cloudpickle.py

Lines 269 to 279 in 31d76ea

    
           class CloudPickler(Pickler): 
        
               dispatch = Pickler.dispatch.copy() 
        
               def __init__(self, file, protocol=None): 
        
                   if protocol is None: 
        
                       protocol = DEFAULT_PROTOCOL 
        
                   Pickler.__init__(self, file, protocol=protocol) 
        
                   # map ids to dictionary. used to ensure that functions can share global env 
        
                   self.globals_ref = {} 
        
                   raise ValueError("Test Bug")

.travis.yml

robertnishihara · 2019-03-17T21:45:56Z

@williamma12 this looks pretty good to me. I left a couple small comments.

robertnishihara · 2019-03-17T21:46:19Z

Also, it looks like some of the non-Ray Travis builds are failing. Can you rebase on the current master?

robertnishihara

Looks good to me, thanks @williamma12!

I think this can be merged. @ogrisel want to take a look? Otherwise, I can merge this sometime tomorrow.

ogrisel

Thanks for your contribution! I am ok to merge this as is but I would be also open to including more long-running ray tests to ensure better coverage of edge cases.

See also other nitpicks below:

.travis.yml

ogrisel · 2019-03-19T13:43:54Z

Thank you very much. I am merging this as this is already a great start. We can always increase the test coverage with other non-flaky test files later.

pierreglaser · 2019-03-19T13:52:31Z

Thanks a lot @williamma12 @robertnishihara!

williamma12 force-pushed the tests/ray_test branch from ad4be0c to 50ac931 Compare March 11, 2019 23:12

robertnishihara reviewed Mar 12, 2019

View reviewed changes

robertnishihara reviewed Mar 13, 2019

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

robertnishihara reviewed Mar 13, 2019

View reviewed changes

robertnishihara reviewed Mar 14, 2019

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

.travis.yml Outdated Show resolved Hide resolved

.travis.yml Outdated Show resolved Hide resolved

.travis.yml Outdated Show resolved Hide resolved

.travis.yml Outdated Show resolved Hide resolved

robertnishihara reviewed Mar 17, 2019

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

.travis.yml Show resolved Hide resolved

robertnishihara reviewed Mar 17, 2019

View reviewed changes

.travis.yml Show resolved Hide resolved

williamma12 added 16 commits March 17, 2019 16:36

Add ray to travis tests

1c4f805

Add more tests

80fcd0c

Update ray dependencies

ddae239

Update travis testing

662b620

minor changes

31b54c1

clone ray but dont install to try to run tests

225c466

Added ray project url

9d01269

Remove pytest args

802cf94

Reorder tensorflow to end of requirements

1f75534

remove pytest arguments

9ebd41d

Reduce python version to 3.5

014da2f

temp

80c46c6

Remove echo

15a1aac

load ray file test

7287daf

Did work

b2d7d6e

bash syntax error

6aed327

williamma12 added 13 commits March 17, 2019 16:36

Add cython requirement for ray [ci downstream]

b2d23bd

Resolve comments [ci ray]

8987064

Give ray its own script build process [ci ray]

1e7c371

Undo unnecessary changes [ci ray]

8c5ee15

Resolve comments [ci ray]

052063a

Syntax errors

9c56777

Try removing extraneous installs for ray [ci ray]

8659726

Syntax error [ci ray]

1155cde

Resolve comments [ci ray]

f85a8fa

Test to make sure that a bug in cloudpickle will happen in ray [ci ray]

f27655c

Temporarily skip cloudpickle tests [ci ray]

fb8292a

Remove "bug" from cloudpickle

f5cc58c

resolve comments [ci ray]

04d1041

williamma12 force-pushed the tests/ray_test branch from 1216e79 to 04d1041 Compare March 17, 2019 23:37

williamma12 added 2 commits March 17, 2019 16:38

same as above [ci ray]

4e1782b

Undo change [ci ray]

bdb9ebe

robertnishihara changed the title ~~MNT: Add ray tests to ci configuration against the development version of cloudpickle~~ Add ray tests to ci configuration against the development version of cloudpickle Mar 18, 2019

robertnishihara reviewed Mar 18, 2019

View reviewed changes

ogrisel approved these changes Mar 18, 2019

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

.travis.yml Outdated Show resolved Hide resolved

.travis.yml Outdated Show resolved Hide resolved

williamma12 and others added 8 commits March 18, 2019 09:23

Resolve comments [ci ray]

5b0f319

typo [ci ray]

f92a79b

I finally learned how to copy paste code correctly [ci ray]

dbd652b

Update .travis.yml

dc544ac

Update [ci ray]

6732c1c

Update [ci ray]

02ae007

update [ci ray]

8ec294f

Update [ci ray]

5edfb20

ogrisel merged commit 810c3f4 into cloudpipe:master Mar 19, 2019

	install:
	- pip install .
	- pip install --upgrade -r dev-requirements.txt
	- pip install tornado
	- if [[ $TRAVIS_PYTHON_VERSION != 'pypy'* ]]; then
	pip install numpy scipy;
	fi
	- if [[ $PROJECT != "" ]]; then

Add ray tests to ci configuration against the development version of cloudpickle #256

Add ray tests to ci configuration against the development version of cloudpickle #256

Uh oh!

Conversation

williamma12 commented Mar 11, 2019

Uh oh!

codecov bot commented Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codecov bot commented Mar 11, 2019

Codecov Report

Uh oh!

robertnishihara left a comment

Choose a reason for hiding this comment

Uh oh!

robertnishihara Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

williamma12 commented Mar 12, 2019

Uh oh!

Uh oh!

robertnishihara Mar 13, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

robertnishihara Mar 13, 2019

Choose a reason for hiding this comment

Uh oh!

williamma12 Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertnishihara Mar 13, 2019

Choose a reason for hiding this comment

Uh oh!

robertnishihara Mar 13, 2019

Choose a reason for hiding this comment

Uh oh!

williamma12 Mar 13, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robertnishihara commented Mar 14, 2019

Uh oh!

williamma12 commented Mar 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robertnishihara commented Mar 17, 2019

Uh oh!

robertnishihara commented Mar 17, 2019

Uh oh!

robertnishihara left a comment

Choose a reason for hiding this comment

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ogrisel commented Mar 19, 2019

Uh oh!

pierreglaser commented Mar 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

codecov bot commented Mar 11, 2019 •

edited

Loading

williamma12 Mar 13, 2019 •

edited

Loading

williamma12 commented Mar 16, 2019 •

edited

Loading