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 21 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
5 changes: 5 additions & 0 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
# Copyright (c) 2024, NVIDIA CORPORATION.

exclude: |
(?x)^(
tests/templates/.*py
)$

repos:
- repo: https://github.com/pre-commit/pre-commit-hooks
rev: v4.6.0
Expand Down
22 changes: 22 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,28 @@ Any option without a default is required.

- How should we split up build requirements between `build-system` and `tool.rapids-build-backend`? In theory any dependency that doesn't need suffixing could also go into `build-system.requires`. I think it's easier to teach that all dependencies other than `rapids-build-backend` itself should to into `tool.rapids-build-backend`, but I don't know how others feel.

## `setuptools` support
jameslamb marked this conversation as resolved.
Show resolved Hide resolved

This project supports builds using `setuptools.build_meta` as their build backend, and which use a `setup.py` for configuration.

However, it does not support passing a list of dependencies through `setup_requires` to `setuptools.setup()`.
If you're interested in using `setuptools.build_meta` and a `setup.py`, pass a list of dependencies that need to be installed prior to `setup.py` running through `rapids-build-backend`'s requirements, like this:

```toml
[project]
build-backend = "rapids_build_backend.build"
requires = [
"rapids-build-backend",
"setuptools"
]

[tool.rapids-build-backend]
build-backend = "setuptools.build_meta"
requires = [
"Cython"
]
```

## Rejected ideas

- We could also include the rewrite of VERSION that we use for RAPIDS builds, but this is really more specific to our release process than the general process of building our wheels. I don't think someone building a wheel locally should see the same version as what we produce via CI. If we really wanted we could pull dunamai as a dependency and write a different version here, though.
71 changes: 59 additions & 12 deletions rapids_build_backend/impls.py
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,35 @@ 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:
raise ValueError(
"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."
)


# 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 +263,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 +297,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 +320,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()
32 changes: 32 additions & 0 deletions tests/templates/dependencies-rbb-only.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
# Copyright (c) 2024, NVIDIA CORPORATION.

# [description]
#
# dependencies.yaml that intentionally only updates [tool.rapids-build-backend] table
# in pyproject.toml.
#
# Create new templates to test other dependencies.yaml contents.
#

files:
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
5 changes: 5 additions & 0 deletions tests/templates/setup.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
from setuptools import setup

{% for line in setup_py_lines %}
{{ line }}
{% endfor %}
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
123 changes: 123 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,7 @@

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

Expand Down Expand Up @@ -81,6 +82,128 @@ 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,
):
package_dir = tmp_path / "pkg"
os.makedirs(package_dir)

template_args = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit: This is my fault for creating this pattern (I assume it was me), but on second read it's certainly a bit strange to use the same template args for all the generate_from_template calls. It would probably be cleaner to have just the necessary ones for each file.

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 as far as I can tell, there's 0 sharing of template arguments across the files, so it should be possible to instead do something like:

template_values = {
   "pyproject.toml": {
       "name": "something",
       "build_backend": "setuptools.build_meta
   },
   "dependencies.yaml": {
       "dependencies": {
            "cu11": ["cupy-cuda11x>=12.0.0"],
            "cu12": ["cupy-cuda12x>=12.0.0"],
        }
   }
}
generate_from_template(package_dir, template_args)

Where the top-level keys are the files to use and the dictionary passed after each one is the template arguments.

However... I'd rather not do that refactoring as part of this PR. I don't think that's THAT much better than the current state, at least with the small number of tests we currently have here.

"name": "setuptools-with-imports-in-setup-py",
"build_backend": "setuptools.build_meta",
"build_backend_package": "setuptools",
"flags": {
"commit-files": "[]",
"dependencies-file": '"dependencies-rbb-only.yaml"',
},
"setup_py_lines": [
"import more_itertools",
"",
"print(more_itertools.__version__)",
"",
"setup()",
],
}
generate_from_template(package_dir, "dependencies-rbb-only.yaml", template_args)
generate_from_template(package_dir, "pyproject.toml", template_args)
generate_from_template(package_dir, "setup.py", template_args)

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, capfd
):
package_dir = tmp_path / "pkg"
os.makedirs(package_dir)

template_args = {
"name": "setuptools-with-imports-in-setup-py",
"build_backend": "setuptools.build_meta",
"build_backend_package": "setuptools",
"flags": {
"commit-files": "[]",
"dependencies-file": '"dependencies-rbb-only.yaml"',
},
"setup_py_lines": [
"import more_itertools",
"",
"print(more_itertools.__version__)",
"",
"setup()",
],
}
generate_from_template(package_dir, "dependencies-rbb-only.yaml", template_args)
generate_from_template(package_dir, "pyproject.toml", template_args)
generate_from_template(package_dir, "setup.py", template_args)

# 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, capfd
):
package_dir = tmp_path / "pkg"
os.makedirs(package_dir)
template_args = {
"name": "setuptools-with-imports-in-setup-py",
"build_backend": "setuptools.build_meta",
"build_backend_package": "setuptools",
"flags": {
"commit-files": "[]",
"dependencies-file": '"dependencies-rbb-only.yaml"',
},
"setup_py_lines": [
"import more_itertools",
"",
"print(more_itertools.__version__)",
"",
"setup(",
" setup_requires=['more-itertools'],",
")",
],
}
generate_from_template(package_dir, "dependencies-rbb-only.yaml", template_args)
generate_from_template(package_dir, "pyproject.toml", template_args)
generate_from_template(package_dir, "setup.py", template_args)

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