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

skip get_requires for setuptools projects #41

Merged
merged 24 commits into from
Jun 12, 2024
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 60 additions & 12 deletions rapids_build_backend/impls.py
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,36 @@ def _edit_pyproject(config):
shutil.move(bkp_pyproject_file, pyproject_file)


def _check_setup_py(setup_py_contents: str) -> None:
"""
``setuptools.get_requires_for_build_wheel()`` executes setup.py if it exists,
to check for dependencies in ``setup_requires`` (passed to ``setuptools.setup()``).

That's a problem for rapids-build-backend, as at the point where that's invoked,
its recalculated list of build dependencies (modified in ``_edit_pyproject()``)
haven't yet been installed.

If any of them are imported in ``setup.py``, those imports will fail.

This function raises an exception if it detects ``setup_requires`` being used in
a ``setup.py``, to clarify that ``rapids-build-backend`` can't support that case.

ref: https://github.com/rapidsai/rapids-build-backend/issues/39
"""

# pattern = "any use of 'setup_requires' on a line that isn't a comment"
setup_requires_pat = r"^(?!\s*#+).*setup_requires"
vyasr marked this conversation as resolved.
Show resolved Hide resolved

if re.search(setup_requires_pat, setup_py_contents, re.M) is not None:
error_msg = (
"Detected use of 'setup_requires' in a setup.py file. "
"rapids-build-backend does not support this pattern. Try moving "
"that list of dependencies into the 'requires' list in the "
"[tool.rapids-build-backend] table in pyproject.toml."
)
raise ValueError(error_msg)
jameslamb marked this conversation as resolved.
Show resolved Hide resolved


# The hooks in this file could be defined more programmatically by iterating over the
# backend's attributes, but it's simpler to just define them explicitly and avoids any
# potential issues with assuming the right pyproject.toml is readable at import time (we
Expand All @@ -234,11 +264,23 @@ def get_requires_for_build_wheel(config_settings):
backend := _get_backend(config.build_backend),
"get_requires_for_build_wheel",
):
requires.extend(
backend.get_requires_for_build_wheel(
_remove_rapidsai_from_config(config_settings)
if config.build_backend == "setuptools.build_meta":
_check_setup_py(setup_py_contents=utils._get_setup_py())
# prior to https://github.com/pypa/setuptools/pull/4369 (May 2024),
vyasr marked this conversation as resolved.
Show resolved Hide resolved
# setuptools.build_meta.get_requires_for_build_wheel() automatically
# added 'wheel' to the build requirements. Adding that manually here,
# since this code block skips running
# setuptools.build_meta.get_requires_for_build_wheel().
#
# Without this, running 'pip wheel' might result in an error like
# "error: invalid command 'bdist_wheel'".
requires.extend(["wheel"])
else:
requires.extend(
backend.get_requires_for_build_wheel(
_remove_rapidsai_from_config(config_settings)
)
)
)

return requires

Expand All @@ -256,11 +298,14 @@ def get_requires_for_build_sdist(config_settings):
backend := _get_backend(config.build_backend),
"get_requires_for_build_sdist",
):
requires.extend(
backend.get_requires_for_build_sdist(
_remove_rapidsai_from_config(config_settings)
if config.build_backend == "setuptools.build_meta":
_check_setup_py(setup_py_contents=utils._get_setup_py())
else:
requires.extend(
backend.get_requires_for_build_sdist(
_remove_rapidsai_from_config(config_settings)
)
)
)

return requires

Expand All @@ -276,11 +321,14 @@ def get_requires_for_build_editable(config_settings):
backend := _get_backend(config.build_backend),
"get_requires_for_build_editable",
):
requires.extend(
backend.get_requires_for_build_editable(
_remove_rapidsai_from_config(config_settings)
if config.build_backend == "setuptools.build_meta":
_check_setup_py(setup_py_contents=utils._get_setup_py())
else:
requires.extend(
backend.get_requires_for_build_editable(
_remove_rapidsai_from_config(config_settings)
)
)
)

return requires

Expand Down
18 changes: 18 additions & 0 deletions rapids_build_backend/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,21 @@ def _get_pyproject(dirname: str = ".") -> tomlkit.toml_document.TOMLDocument:
"""Parse and return the pyproject.toml file."""
with open(os.path.join(dirname, "pyproject.toml")) as f:
return tomlkit.load(f)


def _get_setup_py() -> str:
"""
Returns a string with the contents of setup.py,
or empty string if it doesn't exist.
"""
# setuptools.build_meta.get_requires_for_wheel() assumes that "setup.py" is directly
# relative to the current working directly, so rapids-build-backend can too.
#
# ref: https://github.com/pypa/setuptools/blob/f91fa3d9fc7262e0467e4b2f84fe463f8f8d23cf/setuptools/build_meta.py#L304
setup_py_file = "setup.py"
vyasr marked this conversation as resolved.
Show resolved Hide resolved

if not os.path.isfile(setup_py_file):
return ""

with open(setup_py_file) as f:
return f.read()
6 changes: 6 additions & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,12 @@ def _create_nvcc(nvcc_version):
return fn


@pytest.fixture
def examples_dir():
"""Directory with test project files."""
return Path(__file__).parent / "examples"


@contextmanager
def patch_nvcc_if_needed(nvcc_version):
"""Patch the PATH to insert a spoofed nvcc that returns the desired version."""
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# Copyright (c) 2024, NVIDIA CORPORATION.

files:
vyasr marked this conversation as resolved.
Show resolved Hide resolved
py_rapids_build:
output: pyproject
pyproject_dir: .
extras:
table: tool.rapids-build-backend
key: requires
includes:
- build_python
dependencies:
build_python:
specific:
- output_types: [pyproject, requirements]
matrices:
- matrix: {cuda: "85.*"}
packages:
- more-itertools
# keeping this empty means it'll only be filled in if
# rapids-build-backend actually resolves one of the CUDA-specific
# matrices
- matrix: null
packages: null
19 changes: 19 additions & 0 deletions tests/examples/setuptools-with-imports-in-setup-py/pyproject.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# Copyright (c) 2024, NVIDIA CORPORATION.

[build-system]
build-backend = "rapids_build_backend.build"
requires = [
"rapids-build-backend",
"setuptools",
]

[project]
name = "setuptools-with-imports-in-setup-py"
version = "1.2.3"

[tool.rapids-build-backend]
build-backend = "setuptools.build_meta"
commit-files = []
dependencies-file = "dependencies.yaml"
requires = [
] # This list was generated by `rapids-dependency-file-generator`. To make changes, edit dependencies.yaml and run `rapids-dependency-file-generator`.
11 changes: 11 additions & 0 deletions tests/examples/setuptools-with-imports-in-setup-py/setup.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# Copyright (c) 2024, NVIDIA CORPORATION.

# Note that more-itertools is not listed anywhere in pyproject.toml.
# This import can only succeed if rapids-build-backend successfully identified it as
# a build-time dependency via dependencies.yaml.
import more_itertools
from setuptools import setup

print(more_itertools.__version__)

setup()
36 changes: 36 additions & 0 deletions tests/test_impls.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import pytest

from rapids_build_backend.impls import (
_check_setup_py,
_edit_pyproject,
_get_cuda_suffix,
_remove_rapidsai_from_config,
Expand Down Expand Up @@ -322,3 +323,38 @@ def test_edit_pyproject(

with open("pyproject.toml") as f:
assert f.read() == original_contents


@pytest.mark.parametrize(
["setup_py_content", "expect_error"],
[
("", False),
("from setuptools import setup\n\nsetup()\n", False),
# 'setup_requires' in a comment on its own line
("from setuptools import setup\n# setup_requires\n\nsetup()\n", False),
# 'setup_requires' actually passed into setup(), on the same line
("from setuptools import setup\nsetup(setup_requires=[])\n", True),
# 'setup_requires' actually passed into setup(), on its own line
(
"from setuptools import setup\nsetup(\n setup_requires=['rmm']\n)\n# setup_requires\n", # noqa: E501
True,
),
# 'setup_requires' actually passed into setup(), via a dictionary
(
"from setuptools import setup\nopts={'setup_requires': ['rmm']}\nsetup(**opts)\n", # noqa: E501
True,
),
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big deal, but this parametrization is hard to read. You might consider doing something like creating a list of multiline strings or something and then using zip(contents, errors) instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah you're right. I just tried reformatting it by removing "should this raise an error?" from the parameterization completely and splitting it into 2 tests (one with cases that shouldn't result in an error, one with cases that should).

I think it's a lot easier to read and understand now:

image

],
)
@patch("rapids_build_backend.impls._get_git_commit", Mock(return_value="abc123"))
jameslamb marked this conversation as resolved.
Show resolved Hide resolved
def test_check_setup_py(
setup_py_content,
expect_error,
):
if expect_error:
with pytest.raises(
ValueError, match=r"Detected use of 'setup_requires' in a setup\.py file"
):
_check_setup_py(setup_py_content)
else:
_check_setup_py(setup_py_content) is None
71 changes: 71 additions & 0 deletions tests/test_packages.py
Copy link
Contributor

Choose a reason for hiding this comment

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

The three new tests are similar enough that I suspect you could convert them to a single parametrized test fairly easily, but I could be wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the sets of assertions are different enough, and the setup before that small enough, to justify separate test cases.

They could get put into one parameterized test but that test would end like this:

if first_thing_being tested:
   # assertions
elif second_thing_being_testing:
   # other assertions
else:
   # third set of assertions

I think it's a little easier to follow the logic by keeping these new tests as separate cases.

Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import glob
import os
import shutil
import subprocess
import zipfile
from email.parser import BytesParser

Expand Down Expand Up @@ -81,6 +83,75 @@ def test_simple_setuptools(tmp_path, env, nvcc_version):
assert extras == {"test": {"dask-cuda==24.4.*,>=0.0.0a0"}}


# rapids-build-backend should support projects using setuptools whose setup.py
# file has 'import' statements depending on some project(s) that need to be extracted
# from dependencies.yaml at build time
def test_setuptools_with_imports_in_setup_py_works(
tmp_path, isolated_env, examples_dir
):
package_dir = tmp_path / "pkg"
shutil.copytree(
src=examples_dir / "setuptools-with-imports-in-setup-py", dst=package_dir
)

with patch_nvcc_if_needed(nvcc_version="85"):
name, build_requires, requirements, extras = _generate_wheel(
env=isolated_env, package_dir=package_dir
)

assert name == "setuptools-with-imports-in-setup-py-cu85"
assert {"more-itertools"}.issubset(build_requires)
assert requirements == set()


def test_setuptools_with_imports_in_setup_py_fails_on_missing_imports(
tmp_path, isolated_env, examples_dir, capfd
):
package_dir = tmp_path / "pkg"
shutil.copytree(
src=examples_dir / "setuptools-with-imports-in-setup-py", dst=package_dir
)

# only the CUDA '85.*' in this example provides required build dependency
# 'more-itertools', so it won't be found if using some other matrix.
#
# This test confirms that rapids-build-backend fails loudly in that case, instead of
# silently ignoring it.
#
# It'd also catch the case where other tests accidentally pass because
# 'more-itertools' already existed in the environment where tests run.
with patch_nvcc_if_needed(nvcc_version="25"):
vyasr marked this conversation as resolved.
Show resolved Hide resolved
with pytest.raises(subprocess.CalledProcessError, match=".*pip.*"):
_generate_wheel(env=isolated_env, package_dir=package_dir)

captured_output = capfd.readouterr()
assert (
"ModuleNotFoundError: No module named 'more_itertools'" in captured_output.out
)


def test_setuptools_with_setup_requires_fails_with_informative_error(
tmp_path, isolated_env, examples_dir, capfd
):
package_dir = tmp_path / "pkg"
shutil.copytree(
src=examples_dir / "setuptools-with-imports-in-setup-py", dst=package_dir
)

with open(package_dir / "setup.py", "w") as f:
f.write("from setuptools import setup\nsetup(setup_requires=['Cython'])\n")

with patch_nvcc_if_needed(nvcc_version="85"):
with pytest.raises(subprocess.CalledProcessError, match=".*pip.*"):
_generate_wheel(env=isolated_env, package_dir=package_dir)

captured_output = capfd.readouterr()
assert (
"ValueError: Detected use of 'setup_requires' in a setup.py file"
in captured_output.out
)


@pytest.mark.parametrize("nvcc_version", ["11", "12"])
def test_simple_scikit_build_core(tmp_path, env, nvcc_version):
template_args = {
Expand Down