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.

Sign up for GitHub

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

Jump to bottom

Ship Modin with Ray. #3109

Merged
merged 20 commits into from
Nov 29, 2018
Merged

Ship Modin with Ray. #3109

merged 20 commits into from
Nov 29, 2018

Conversation

devin-petersohn
Copy link
Member

What do these changes do?

Adds modin to the build process for Ray

Related issue number

Resolves #3108

@devin-petersohn
Copy link
Member Author

cc @robertnishihara @pcmoritz I am not sure if this will also make it in the wheels.

robertnishihara
robertnishihara reviewed Oct 22, 2018
View reviewed changes
thirdparty/scripts/setup.sh Outdated
##############################################
# modin
##############################################
bash "$TP_SCRIPT_DIR/build_modin.sh"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if this is the right way to do this, since it seems like we're deprecating this file, though I'm not sure. @chuxi can you comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am certainly willing to move this, but I am not sure where it will go.

@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

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

@robertnishihara robertnishihara changed the title Ship modin Ship Modin with Ray. Oct 25, 2018
@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

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

@devin-petersohn
Copy link
Member Author

This will not work because of the use_raylet parameter being dropped from the ray.init API. Is there any chance to add it back for one release, even though it is not doing anything? cc @robertnishihara

@atumanov
Copy link
Contributor

Hey @devin-petersohn , could you elaborate on this, why do you need use_raylet to be there even though it will be a no-op? Are you concerned about existing binary releases of Modin that currently use ray.init and would automatically start using xray instead? Nothing changes from the application developer's perspective, and ray.init invocation is controlled by the Modin code from what I understand. So, just wondering about context.

@devin-petersohn
Copy link
Member Author

I am not worried about defaulting to xray, we are already using xray. The problem comes with how our binary calls ray.init, because we use ray.init(use_raylet=True). Since that was completely dropped from the API, we get issues like this: modin-project/modin#236

I prefer that we don't break backwards compatibility with Modin, because we have been using xray for a month or so now (and two of our releases use xray).

@atumanov
Copy link
Contributor

I see, the issue is that existing binary releases of Modin are explicitly passing use_raylet. So dropping use_raylet (which I agree is not back-compat) would only be an issue for the Modin releases that are already out, right? Going forward, as Modin ships with Ray (i.e., for this PR), we have full control over synchronizing their versions. Specifying a required ray package version for the modin pypi package is also an option to guarantee that you get the expected ABI.

@devin-petersohn
Copy link
Member Author

Yes, we already specify that requirement. The problem is that we explicitly pull a the most recent binary of Modin into the Ray repo. It's a chicken and egg problem.

Modin depends on the most recent binary of Ray, which currently requires that we have use_raylet if we want to use xray. We can remove that in a subsequent release, but then the people that install modin with pip install modin will end up using legacy Ray, at least until we release again bumping the Ray version we depend on. If we do that, the version shipped with Ray will always lag the pypi binary by 1 release. This is because we have to release a version removing use_raylet so that Ray can release with a working version of Modin, then Modin will have to release again bumping the version of Ray. The easiest way to solve the chicken and egg problem is to release Ray for a single release with use_raylet still as optional in ray.init, but as a no-op (we can even throw an error if someone tries to set use_raylet=False). Then we can safely remove it in a subsequent release without having a release that uses legacy Ray.

@richardliaw
Copy link
Contributor

Maybe keep use_raylet since it's an end-user API change, and raise DeprecationWarning() noting that this will do nothing and will be removed after 0.6 (or some specific point release)?

@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

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

@robertnishihara
Copy link
Collaborator

Maybe use curl instead of wget. If that fails, maybe try upgrading openssl in https://github.com/ray-project/ray/blob/master/python/build-wheel-manylinux1.sh.

@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

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

@pcmoritz pcmoritz merged commit 4d2010a into ray-project:master Nov 29, 2018
@williamma12 williamma12 mentioned this pull request Feb 8, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robertnishihara robertnishihara robertnishihara left review comments

@pcmoritz pcmoritz pcmoritz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ship Modin with Ray
7 participants
@devin-petersohn @AmplabJenkins @atumanov @richardliaw @robertnishihara @suquark @pcmoritz