-
Notifications
You must be signed in to change notification settings - Fork 78
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
Build py-ticking wheel using docker on top of a manylinux base image. #5267
Build py-ticking wheel using docker on top of a manylinux base image. #5267
Conversation
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.
The C++ part LGTM
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've built the wheels and tested every version locally on Fedora 39 using demo_simple.py with success.
I don't envy this build process. I think it will be a challenge to extend this process as-is to Windows and Mac, but that's a problem for another day.
We'll want to add a py.typed marker to all of these wheels, see #5196 for context.
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.
The wheel names LGTM
Added py.typed to setup.py. |
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've got structural concerns about having gradle tasks do the work for all the python versions. Ideally, they would be split up, and could be built and tested independently. It's not an absolute hard requirement IMO; I'm happy to say "good enough" and ship something that works today. I have grand visions of improving the state of doing docker stuff in deephaven-core, and that may becoming more pressing given python testing matrix concerns. #3723
For reference, https://github.com/deephaven/deephaven-server-docker/blob/main/contexts/turbodbc-wheel/Dockerfile is a "complex" dependency that we build (just for testing purposes), and I suspect we would be doing ourselves a favor in the long run if we could eventually express the pydeephaven-ticking build process as "simple" Dockerfiles (/bake files) as opposed to docker-in-gradle. This isn't actionable today, but just FYI.
|
||
def testPyClientTicking = Docker.registerDockerTask(project, 'testPyClientTicking') { | ||
def testPyClientTickingManyLinux = { wheelsSet, taskName, parentContainer, image -> Docker.registerDockerTask(project, taskName ) { |
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.
Some comments about structuring and all-on-one.
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.
We discussed over slack, we want to hear from Colin what he thinks, but otherwise we are willing to live with this for now. Also, for the record, I don't think is obvious that the alternative of building separate images for each python version is better. That's a lot more images, a lot more build time, and a lot more space and noise in the output of docker image ls
.
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.
Pros of more tasks:
- can parallelize, runs "faster"
- can run an individual build at a time by only selecting a single task
Downsides:
- more docker layers
- more docker step invocations
- more cpu cycles (not necessarily longer runtime, if parallel is enabled)
In general, this isn't the kind of task that developers or users are going to run locally, but the focus is going to be on prompt running time in CI (which is non-parallel, or at least has very few apparent cores to work with), so I think this is a good option.
If I were king, I'd ask for a few things:
- Change runCommand to an entrypoint - by being an entrypoint it won't create a new layer. Re-running will be based on if gradle inputs are cached - no need to rely on Docker copying inputs into the daemon and seeing if a checksum has changed, no need to care about the
rm
at the end of the command. - Consider templating the bash script (as an entrypoint) - no new images means no
docker image ls
spam, so we can consider splitting by task.wheelSet
groovy collection to string into bashWHEELS_SET
, split on string into bashspec
, mangle intotag
(and otherwise never use it- avoiding list->string->list parsing/escaping/normalizing surprises for future readers/editors
- mixing bash vars and groovy vars (which excitingly both use
${...}
, based only on...
"
strings vs'
strings nested in each other, etc.- Basically anything to lower the cognitive load of reading (and someday editing) this stuff. No specific hills I would die on (well, as king, make someone else die on), but generally try to take advantage of being an entrypoint.
If you'd like to take that on now, I'd happily review/participate, but I'm also approving the PR right now, happy to have it dealt with next time this is touched.
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.
Thanks, I think it would make sense to get this as-is now if it is above "good enough" threshold, and then get some experience on using it during releases to get the actual work products built. With that on tow we can do a second pass and I can volunteer myself for doing that.
CFLAGS="-I${DHCPP}/include" \ | ||
LDFLAGS="-L${DHCPP}/lib" \ | ||
DEEPHAVEN_VERSION="${DEEPHAVEN_VERSION}" \ | ||
python3 setup.py build_ext -i; \ |
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.
FYI, I see this warning: https://blog.ganssle.io/articles/2021/10/setup-py-deprecated.html.
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.
That's the same warning that you see for py/client
, yah?
as in, we are all in it together? #2233
From the point of view of what this PR is trying to accomplish, I think this qualifies as "no worse than before".
Unless I am misunderstanding your comment, I don't see how this PR could help with that without growing dramatically in scope.
@niloc132 , I'm not going to have this be a sticking point because I'm happy to have something that works whether or not it is pretty to build. Do you agree? |
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.
The Python changes LGTM. Do we need to a follow-up ticket for auto uploading the wheels to PYPI?
The pre-existing tasks work as before but they now use a manylinux docker image as base. This makes the created wheel installable in many linux distributions (as opposed to before it being tied to ubuntu).
The test (testPyDeephavenTicking task) is done on top of ubi8 (RHEL 8) to enforce validation that the wheel produced in one distribution (the manylinux version we are using is debian based) works in another.
There are two task that are intended for release:
py-client-ticking:pyClientTickingAllWheels
builds multiple wheels, one for each python version in 3.8, 3.9, 3.10, 3.11, 3.12. This task is intended for releases, to create wheels that can be uploaded to pypi.py-client-ticking:testPyClientTickingAllWheels
runs the tests on all wheels built by the previous one.