-
Notifications
You must be signed in to change notification settings - Fork 8
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
Conversation
This reverts commit 6fa35e0.
tests/examples/setuptools-with-imports-in-setup-py/dependencies.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: Bradley Dice <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than creating a new examples directory for tests, can you reuse the existing templates? I would expect the existing dependencies.yaml and pyproject.toml files to work out of the box (although maybe you'll have to generalize dependencies.yaml, not sure). Adding setup.py there will allow you to also parametrize the tests so that you can test packages with setup.py in more scenarios (e.g. with setup_requires
, setup_requires
in a comment, etc).
Co-authored-by: Vyas Ramasubramani <[email protected]>
tests/examples/setuptools-with-imports-in-setup-py/dependencies.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bunch of nitpicking on my part, but overall this looks ready to me.
tests/test_impls.py
Outdated
("", 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, | ||
), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
package_dir = tmp_path / "pkg" | ||
os.makedirs(package_dir) | ||
|
||
template_args = { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I pushed a new commit to the |
I just snuck a |
Alright I think all the comments have been addressed, so gonna merge this and keep moving forward. Thanks everyone! |
fixes #39
As of this change
rapids-build-backend
will do the following when asked to build a wheel for a project usingsetuptools
as its build backend:setuptools.build_meta.get_requires_for_*()
methodssetup.py
that is passingsetup_requires
tosetuptools.setup()
See #39 for a lot more details.
Notes for Reviewers
How I tested this
Added new unit tests here.
Built from source on this branch in rapidsai/ucx-py#1044 and saw CI pass...things work as expected, based on a quick scan of the logs there. (ignore the failing docs build there, that'll pass once we've published new
rapids-build-backend
wheels).(ucx-py build link)
Thoughts on releasing
This change is "breaking" in the sense that it changes the behavior in a user-visible way, but not in the practical sense... there are 0 projects currently using
rapids-build-backend
that should be affected by this change.So I think that we should release
rapids-build-backend
with this change in it as v0.3.1, not v0.4.0, to avoid a round of PRs to update the existingrapids-build-backend
pins across projects already using it.