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

[BSE-4203] Use Pixi for Pip Build Deps #46

Merged
merged 11 commits into from
Dec 12, 2024
Merged

[BSE-4203] Use Pixi for Pip Build Deps #46

merged 11 commits into from
Dec 12, 2024

Conversation

srilman
Copy link
Contributor

@srilman srilman commented Dec 11, 2024

Changes included in this PR

Replace the conda-lock usage in our Pip pipelines with Pixi.

We use conda-lock to set up a reproducible environment for our C++ dependencies. However, its not really reproducible because we construct the lockfile during the build process. Furthermore, we install more dependencies than we need for building. We do all this because conda-lock doesn't have the features we need.

Pixi has features like multiple environments that we can use instead. Furthermore, with Pixi, we can only install the C++ dependencies during the Pip build.

Some other changes:

  • Fix a bug with the versioning and the data folder in the Pip package
  • Use the ManyLinux 2.28 image
  • Build MPI4Py from Source and Vendor in Package

Note that we unfortunately need to still use the C++ compilers from Pixi / Conda. Ideally, we should use the compiler provided by cibuildwheel for best compatibility, but we can't until we find other sources for our C++ dependencies. Most are already provided (libcurl on the system, AWS SDK from PyArrow, Boost through CMake build), but HDF5 is the hard one.

Testing strategy

Running the Pip CIs.

User facing changes

Ideally none.

Checklist

  • Pipelines passed before requesting review. To run CI you must include [run CI] in your commit message.
  • I am familiar with the Contributing Guide
  • I have installed + ran pre-commit hooks.

@srilman srilman marked this pull request as ready for review December 11, 2024 23:43
pyproject.toml Outdated
# Increase pip debugging output
build-verbosity = 1
build-frontend = { name = "pip" }
#, args = ["--only-binary", "mpi4py", "--extra-index-url", "https://pypi.anaconda.org/mpi4py/simple"] }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: Remove when addressing comments

needs: build_bodo_linux_wheels
steps:
- uses: actions/download-artifact@v4
id: download-artifact
with:
pattern: cibw-wheels-ubuntu-*
path: .
- uses: actions/setup-python@v5
with:
python-version: ${{ matrix.python_version }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this because I couldn't add set -exo pipefail inside the docker container. Thus, the job kept passing even though the test failed. This way, the job fails is the test fails

pip install setuptools_scm
echo "bodo_version=`python -m setuptools_scm`" >> $GITHUB_OUTPUT
outputs:
bodo_version: ${{ steps.get_version.outputs.bodo_version }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We ran into a problem where cibuildwheel was using the wrong version during build. This fixes that by fetching the version and fixing it beforehand

CPPFLAGS="-isystem /project/.pixi/envs/pip-cpp/include"
CC=/project/.pixi/envs/pip-cpp/bin/x86_64-conda-linux-gnu-gcc
CXX=/project/.pixi/envs/pip-cpp/bin/x86_64-conda-linux-gnu-g++
DISABLE_CCACHE=1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CIBW is annoying and wont combine the envs from CIBW_ENVIRONMENT and CIBW_ENVIRONMENT_LINUX on Linux, so I had to copy them here.

Plus, Pixi shell activation wasn't working inside of the docker container, so I had to manually set envs to find the compiler correctly.

@srilman srilman changed the title [WIP] [BSE-4203] Use Pixi for Pip [BSE-4203] Use Pixi for Pip Deps Dec 12, 2024
@srilman srilman changed the title [BSE-4203] Use Pixi for Pip Deps [BSE-4203] Use Pixi for Pip Build Deps Dec 12, 2024
Copy link
Contributor

@IsaacWarren IsaacWarren left a comment

Choose a reason for hiding this comment

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

Thanks Srinivas this is great!

@@ -151,6 +176,7 @@ jobs:
pip install bodo bodosql
- name: Test
run: |
set -exo pipefail
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
set -exo pipefail
set -eo pipefail

Had issues with this printing tokens

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm actually fixing that in a separate PR. Its a different issue, so we can keep this

@@ -53,6 +76,7 @@ jobs:
pip install bodo
- name: Test
run: |
set -exo pipefail
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
set -exo pipefail
set -eo pipefail

Had issues with this printing tokens

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above

Copy link
Contributor

@hadia206 hadia206 left a comment

Choose a reason for hiding this comment

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

Great job @srilman
Thanks

@@ -34,8 +34,10 @@ def patch_lib(fpath):
# @rpath/libarrow.500.dylib (compatibility version 500.0.0, current version 500.0.0)
# @rpath/libc++.1.dylib (compatibility version 1.0.0, current version 1.0.0)
# /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1311.0.0)
print("Loaded Libs for", fpath)
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to keep prints in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes for sure, it was useful for debugging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And it doesn't cause any issues

@srilman srilman merged commit a6bb272 into main Dec 12, 2024
21 checks passed
@srilman srilman deleted the slade/pixi-pip branch December 12, 2024 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants