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.

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 and publish pydeephaven-ticking wheels #5290

Merged

Conversation

devinrsmith
Copy link
Member

@devinrsmith devinrsmith commented Mar 25, 2024

Fixes #5288

This does not upload the wheels to PyPi; our first release of pydeephaven-ticking to PyPi should be manual, after which point we can create the necessary project tokens and automate the final steps.

Prerequisite for deephaven#5288
stanbrub
stanbrub previously approved these changes Mar 26, 2024
Copy link
Contributor

@stanbrub stanbrub left a comment

Choose a reason for hiding this comment

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

I'm not accounting for what it builds, but it does build. And the workflow looks right. And I see the artifact in the appropriate build directory. (Though the wheel file name is crazy long.)

@devinrsmith devinrsmith marked this pull request as ready for review March 26, 2024 16:04
@devinrsmith
Copy link
Member Author

This unconditionally adds all wheels as part of the build / test process in CI. This may or may not be appropriate.

I would suggest the reasonable alternative is to stick with 1 wheel / test except for nightly / release process. If we want to go this route, it will involve some new build configuration logic.

@devinrsmith devinrsmith requested a review from niloc132 March 26, 2024 16:12
@devinrsmith
Copy link
Member Author

This is currently failing with

publish check failed on py-client-ticking:testPyClientTickingFedoraMakeImage with "No space left of device"

We may need to break up the monolithic build process (all wheels in one docker image) into separate build processes.

'ubi-minimal')
def isCi = System.getenv().getOrDefault("CI", "false") == "true"

def assembleWheelsSet = isCi
Copy link
Member

Choose a reason for hiding this comment

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

Is this logic inverted?

It sounds the "true" case for "Is this CI" has more cases than the "false" case, where I would expect the opposite.

Or rather, what CI? github check on every PR CI? build artifacts for release CI?

Copy link
Member Author

Choose a reason for hiding this comment

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

As written, this will cause any code that is executing in "CI" to have the full wheel set. If we want to limit it, to say "only build (/test) all of the wheels during the nightly build checks, or during release" there is some additional configuration logic we'll need to develop. That isn't something we've sliced-n-diced based on before.

py/client-ticking/build.gradle Outdated Show resolved Hide resolved
py/client-ticking/build.gradle Outdated Show resolved Hide resolved
py/client-ticking/build.gradle Outdated Show resolved Hide resolved
@devinrsmith
Copy link
Member Author

There are errors getting this to run in CI; it appears that the size of the pydeephaven-ticking images cause the runner to run out of disk space.

The standard runners have 14GB of disk space, https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners/about-github-hosted-runners#standard-github-hosted-runners-for-public-repositories.

The pydeephaven-ticking images total ~5.5GB:

REPOSITORY                                                TAG                            IMAGE ID       CREATED          SIZE
deephaven/test-py-client-ticking-ubi-minimal              local-build                    1c85f85120ac   39 seconds ago   1.6GB
deephaven/test-py-client-ticking-fedora                   local-build                    7f286fb53bf7   2 minutes ago    2.56GB
deephaven/py-client-ticking-wheels                        local-build                    a5a04c878ef8   6 minutes ago    3.45GB

, so it's not surprising that could be bump us past our limits.

We may need to investigate alternative strategies for building and testing these wheels. (May be as simple as deleting the docker image after the gradle task is done?)

.github/workflows/publish-ci.yml Show resolved Hide resolved
RELEASE.md Outdated Show resolved Hide resolved
RELEASE.md Outdated Show resolved Hide resolved
@devinrsmith devinrsmith marked this pull request as draft March 27, 2024 18:30
@devinrsmith
Copy link
Member Author

Moved back to draft to acknowledge the issues with the current approach.

mofojed
mofojed previously approved these changes Apr 3, 2024
Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

Looks good to me

@devinrsmith devinrsmith marked this pull request as ready for review April 4, 2024 15:23
@devinrsmith
Copy link
Member Author

This is back up for review.

The ticking wheel build process doesn't actually depend on pydeephaven; so removing pydeephaven and the transitive dependencies (arrow, etc) from that process is a big win in terms of disk space.

The majority of disk-space taking logic has been moved into the entrypoints, which means the logic is executed at the startup of the container and isn't persisted to a docker image layer. The container still has some sort of overlay filesystem, but it gets cleaned up after the container is deleted (after the relevant output files can be copied out). This should not adversely effect cacheability, as gradle should still be caching the output as long as none of the inputs have changed.

There is room a lot of room for maintainability, build, and testing improvements, but that can be left for a later date. I think there is good potential to consider these sorts of future improvements with respect to #3723

@devinrsmith devinrsmith changed the title Add pydeephaven-ticking wheels as build artifacts Build and publish pydeephaven-ticking wheels Apr 4, 2024
jcferretti
jcferretti previously approved these changes Apr 6, 2024
Copy link
Member

@niloc132 niloc132 left a comment

Choose a reason for hiding this comment

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

Quick pass mostly focusing on the gradle wiring.

Why not go all out and split the tasks, so we can omit the rm step, just let the overlayfs be deleted?

py/client-ticking/build.gradle Outdated Show resolved Hide resolved
py/client-ticking/build.gradle Outdated Show resolved Hide resolved
@devinrsmith devinrsmith requested a review from niloc132 April 9, 2024 15:39
@devinrsmith devinrsmith linked an issue Apr 9, 2024 that may be closed by this pull request
@devinrsmith devinrsmith merged commit 5307f52 into deephaven:main Apr 9, 2024
15 checks passed
@devinrsmith devinrsmith deleted the add-pydeephaven-ticking-artifacts branch April 9, 2024 17:51
@github-actions github-actions bot locked and limited conversation to collaborators Apr 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
6 participants