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

Remove MANIFEST.in use auto-generated one for sdists and package_data for wheels #1233

Merged
merged 4 commits into from
Mar 16, 2023

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Mar 16, 2023

Description

Using MANIFEST.in currently runs into a pretty nasty scikit-build bug (scikit-build/scikit-build#886) that results in any file included by the manifest being copied from the install tree back into the source tree whenever an in place build occurs after an install, overwriting any local changes. We need an alternative approach to ensure that all necessary files are included in built packages. There are two types:

  • sdists: scikit-build automatically generates a manifest during sdist generation if we don't provide one, and that manifest is reliably complete. It contains all files needed for a source build up to the rmm C++ code (which has always been true and is something we can come back to improving later if desired).
  • wheels: The autogenerated manifest is not used during wheel generation because the manifest generation hook is not invoked during wheel builds, so to include data in the wheels we must provide the package_data argument to setup. In this case we do not need to include CMake or pyx files because the result does not need to be possible to build from, it just needs pxd files for other packages to cimport if desired.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@vyasr vyasr added bug Something isn't working 3 - Ready for review Ready for review by team non-breaking Non-breaking change labels Mar 16, 2023
@vyasr vyasr self-assigned this Mar 16, 2023
@vyasr vyasr requested a review from a team as a code owner March 16, 2023 19:25
@github-actions github-actions bot added the Python Related to RMM Python API label Mar 16, 2023
@bdice
Copy link
Contributor

bdice commented Mar 16, 2023

@vyasr Can you cross-reference this with a bug report in scikit-build?

@vyasr
Copy link
Contributor Author

vyasr commented Mar 16, 2023

@vyasr Can you cross-reference this with a bug report in scikit-build?

Where do you want it? In the PR or somewhere in the source (and if in the source, where)?

@bdice
Copy link
Contributor

bdice commented Mar 16, 2023

Just link it in this PR, please.

@bdice
Copy link
Contributor

bdice commented Mar 16, 2023

Do sdists work? Do we still ship all the pyx/pxd/CMake files in wheels? If so, I can approve.

edit: Nevermind, I see this in the PR description. I assume “all the necessary files” includes these.

Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Please cross-link an issue in scikit-build, then this should be good to go.

@vyasr
Copy link
Contributor Author

vyasr commented Mar 16, 2023

Strictly speaking sdists never worked, and they still don't, in the sense that produced sdists aren't sufficient to build with because they do not include the C++ component of rmm. However that isn't something that's in scope for fixing in this PR.

It looks like my original solution here doesn't quite work though unfortunately. Generated sdists do contain the correct files from the autogenerated manifest, but wheels do not respect this manifest. That is disappointingly different from how manually provided manifests work, and means that we do still have to write package_data. I think my local builds were observing different results due to cached files in the generated distribution from prior builds before I deleted the manifest. It is particularly confusing because if sdists are built when no manifest is present a MANIFEST file is automatically generated into the source tree. Now I am wiping every generated directory locally and still seeing the expected output files, so this approach seems to be the best we can do. It produces both reasonable sdists (via the autogenerated manifest) and wheels (via the specified package data).

@vyasr vyasr changed the title Remove MANIFEST.in and rely on auto-generated one from scikit-build. Remove MANIFEST.in use auto-generated one for sdists and package_data for wheels Mar 16, 2023
@vyasr
Copy link
Contributor Author

vyasr commented Mar 16, 2023

OK I've verified that the packages being generated (both wheels and conda) contain the pxd files now.

@vyasr
Copy link
Contributor Author

vyasr commented Mar 16, 2023

/merge

@rapids-bot rapids-bot bot merged commit d26ebfc into rapidsai:branch-23.04 Mar 16, 2023
@vyasr vyasr deleted the fix/manifest branch March 16, 2023 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for review Ready for review by team bug Something isn't working non-breaking Non-breaking change Python Related to RMM Python API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants