LLNL / hatchet

Graph-indexed Pandas DataFrames for analyzing hierarchical performance data
https://llnl-hatchet.readthedocs.io
MIT License
27 stars 18 forks source link

Add missing cython dependency #125

Closed Jokeren closed 3 months ago

Jokeren commented 4 months ago

Otherwise pip install will ignore cython

Jokeren commented 4 months ago

Hi @cscully-allison and @ilumsden, could you please let me know if there's any comments for this PR?

ilumsden commented 4 months ago

Hi @Jokeren. Sorry for not responding. I was in the process of relocating to Livermore for the summer.

The changes in this PR should not be necessary to add Cython. Hatchet and Thicket are both designed to be PEP 517 compliant. As a result, the process of installing Cython should be taken care of by pip based on the contents of pyproject.toml.

I also reviewed the GitHub Actions logs that you shared with me. Based on those logs, it looks like there are two main issues going on:

  1. pip is rejecting the wheels that we provide on PyPI (which have the Cython code pre-compiled) due to a Python version mismatch
  2. pip is not correctly handling pyproject.toml. More specifically, it parses the file but does not run the step to install the build system dependencies (namely Cython)

Given these issues, I have three questions:

  1. What version of pip are you using?
  2. What version of Python are you using?
  3. Given that the CI is self hosted, what is the architecture of the system this is running on?
Jokeren commented 4 months ago

pip is not correctly handling pyproject.toml. More specifically, it parses the file but does not run the step to install the build system dependencies (namely Cython)

Yeah, I think this is probably the problem.

Jokeren commented 4 months ago

What version of pip are you using?

pip 22.0.2

What version of Python are you using?

Python 3.10.12

Given that the CI is self hosted, what is the architecture of the system this is running on?\

The problem appears on a several systems not only the CI machine.

My local WSL is Ubuntu 22.04. The remote CI is also an Ubuntu 22.04 I suppose.

ilumsden commented 4 months ago

@Jokeren can you try adding the --use-pep517 flag to your pip install command?

Jokeren commented 4 months ago

That doesn't seem to install the cython package

Jokeren commented 4 months ago

pip is rejecting the wheels that we provide on PyPI (which have the Cython code pre-compiled) due to a Python version mismatch

Oh, I guess it might be that the python version on our CI node is too new? Let me ask

Jokeren commented 4 months ago

The remote CI is also an Ubuntu 22.04 I suppose.

I was wrong. The CI was Ubuntu 22.04 but got updated to 24.04. And the python version is 3.12.

ilumsden commented 3 months ago

@Jokeren sorry for the delay, but I might have an idea as to why Cython's not getting installed. Can you try running with both the --use-pep517 and the --check-build-dependencies flags?

ilumsden commented 3 months ago

I've finally managed to reproduce the issue, but, given the cause, I think it's fair to say that this is not a bug in Hatchet.

The issue was encountered when running pip install --no-build-isolation llnl-hatchet. Normally, per PEPs 517 and 518, pip will do the following during installation (particularly of sdists):

  1. Download the source distribution (sdist, usually a tar.gz) or binary distribution (wheel) from the package index (usually PyPI)
  2. Unpack the sdist or wheel
  3. Create a virtual environment
  4. Read the build-system table of pyproject.toml to get build system dependencies
  5. Install build system dependencies
  6. If using a sdist, run the build_wheel function of the build backend specified in the build-system table of pyproject.toml. This creates a local wheel file
  7. Destroy the virtual environment
  8. Move the contents of the downloaded or created wheel to the appropriate install directory (often site-packages)
  9. Delete any remaining artifacts

Steps 3 through 7 of this process are referred to as "build isolation". So, when you provide the --no-build-isolation flag to pip install, pip will skip those steps. As a result, any build dependencies will not be automatically installed by pip. Per the pip team, this is done to prevent disruptions to the user's environment. As a result, it is expected that, when a user provides --no-build-isolation, it is their responsibility to manually install the build dependencies.

For Hatchet, our build dependencies are setuptools and Cython (see pyproject.toml). As a result, if a user runs pip install --no-build-isolation llnl-hatchet, it is generally considered the users' responsibility to install those packages.

Given all of this, I feel like there's not really anything we could do to fix this without forcing Hatchet to run into the same "disrupting user's environment" issue that pip is trying to avoid.

@pearce8 what is your opinion on this?

Jokeren commented 3 months ago

@ilumsden @pearce8 we just removed --no-build-isolation in the CI, but recommend users as a tip to speedup compilation.

Thanks for investigating!

ilumsden commented 3 months ago

Of course. Happy to help!

I also wanted to share this @Jokeren: https://github.com/pypa/pip/issues/11440

Apparently, there is planned feature for pip to allow users to just build install or build dependencies for a package. With that feature, it will be possible to easily handle the build dependencies with:

$ python3 -m pip install --only-build-deps llnl-hatchet
$ python3 -m pip install --no-build-isolation llnl-hatchet

In terms of stuff that already exists, the pip-compile command from pip-tools can already support this with it's --build-deps-for, --all-build-deps, and --only-build-deps flags: https://pip-tools.readthedocs.io/en/latest/cli/pip-compile/

ilumsden commented 3 months ago

Actually, @Jokeren, it sounds like that pip feature may be delayed. It sounds like any implementation of that feature will be dependent on the in-progress PEP 735

ilumsden commented 3 months ago

Since removing --no-build-isolation seems to have fixed the issue in Triton, I'm going to go ahead and close this.