Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add missing cython dependency #125

Closed
wants to merge 1 commit into from
Closed

Conversation

Jokeren
Copy link

@Jokeren Jokeren commented May 10, 2024

Otherwise pip install will ignore cython

Otherwise pip install will ignore cython
@Jokeren
Copy link
Author

Jokeren commented May 11, 2024

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

@ilumsden
Copy link
Collaborator

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
Copy link
Author

Jokeren commented May 13, 2024

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
Copy link
Author

Jokeren commented May 13, 2024

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
Copy link
Collaborator

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

@Jokeren
Copy link
Author

Jokeren commented May 13, 2024

That doesn't seem to install the cython package

@Jokeren
Copy link
Author

Jokeren commented May 13, 2024

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
Copy link
Author

Jokeren commented May 13, 2024

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.

@pearce8 pearce8 added priority-high High priority issues and PRs area-cython Issues and PRs involving Hatchet's cython code type-bug Identifies bugs in issues and identifies bug fixes in PRs labels May 16, 2024
@ilumsden
Copy link
Collaborator

@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
Copy link
Collaborator

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 added a commit to triton-lang/triton that referenced this pull request May 24, 2024
LLNL/hatchet#125 (comment)

Previously `no-build-isolation` is used to take advantage of the
incremental build.
Now that we have containerized the CI, it probably doesn't help much.
@Jokeren
Copy link
Author

Jokeren commented May 24, 2024

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

Thanks for investigating!

@ilumsden
Copy link
Collaborator

Of course. Happy to help!

I also wanted to share this @Jokeren: pypa/pip#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
Copy link
Collaborator

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
Copy link
Collaborator

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

@ilumsden ilumsden closed this May 24, 2024
@ilumsden ilumsden added the status-wont-fix Issue will not be addressed by a PR or this project label May 24, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
area-cython Issues and PRs involving Hatchet's cython code priority-high High priority issues and PRs status-wont-fix Issue will not be addressed by a PR or this project type-bug Identifies bugs in issues and identifies bug fixes in PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants