-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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
[core] Don't build cpp api on pip install #50499
Conversation
@@ -33,6 +33,7 @@ | |||
|
|||
ROOT_DIR = os.path.dirname(__file__) | |||
BUILD_JAVA = os.getenv("RAY_INSTALL_JAVA") == "1" | |||
BUILD_CPP = os.getenv("RAY_INSTALL_CPP") == "1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will break release wheel building? at least this is a behavior change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 123 to 124 in 02d3f3c
if os.getenv("RAY_INSTALL_CPP") == "1": | |
# "ray-cpp" wheel package. |
wouldn't cpp wheel building go down this path anyways and RAY_INSTALL_CPP has to be true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah.. you are right that when we are building for releases, we always set RAY_INSTALL_CPP=1
already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah.. you are right that when we are building for releases, we always set
RAY_INSTALL_CPP=1
already.
well, no, actually, RAY_INSTALL_CPP
is used to toggle if to build the ray
package or the ray-cpp
package. it is not for controlling the cpp header files are built.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what cpp header files? we should only build the cpp api for the ray-cpp package right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what cpp header files?
whatever //cpp:ray_cpp_pkg
is building.
what I meant is that, for example, before this change:
python setup.py bdist_wheel
RAY_INSTALL_CPP=1 SKIP_BAZEL_BUILD=1 python setup.py bdist_wheel
this can correctly generate both the related ray
and ray-cpp
wheel
after your change, it can no longer generate the right ray-cpp
wheel.
The way we actually build the wheels is with:
python setup.py bdist_wheel
RAY_INSTALL_CPP=1 python setup.py bdist_wheel
if there are any file that is generated by //cpp:ray_cpp_pkg
but packaged in ray
wheel, and if it is critical for some reason, the file will be lost now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 204 to 205 in a8e217d
"//cpp:counter.so", | |
"//cpp:plus.so", |
Line 108 in a8e217d
visibility = ["//cpp:__subpackages__"], |
this is really the only place i see something from //cpp mentioned outside. If there's any files generated outside we'd see build failures or something anyways right
And there's no python api/utility or anything exposed from inside cpp/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
talked to @dayshah , will check if this change affects wheel building result . let me know.
Signed-off-by: dayshah <dhyey2019@gmail.com>
Signed-off-by: dayshah <dhyey2019@gmail.com>
@aslonnie tried it out doesn't affect the standard wheel building result for the standard non-cpp ray wheel built on manylinux from directions here https://github.com/ray-project/ray/blob/a692d9bc601e800b758424bf94aee44cb8082955/python/README-building-wheels.md this is the result for with this change vs. without this change
|
why it affects the version of this package? looks like it is because they just released a new version recently.. |
Don't want to build the cpp api when using the pip install -e . --verbose command --------- Signed-off-by: dayshah <dhyey2019@gmail.com> Signed-off-by: Abrar Sheikh <abrar@anyscale.com>
Why are these changes needed?
Don't want to build the cpp api when using the
pip install -e . --verbose
commandRelated issue number
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.