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 explicit declare of package-data #4018

Draft
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented Aug 26, 2024

Summary

@DanielYang59 DanielYang59 changed the title Remove explicit declare of package-data during build Remove explicit declare of package-data Aug 26, 2024
pyproject.toml Outdated Show resolved Hide resolved
@DanielYang59 DanielYang59 marked this pull request as ready for review September 7, 2024 04:10
@DanielYang59 DanielYang59 marked this pull request as draft September 7, 2024 04:19
@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Sep 7, 2024

UPDATE: ignore the following, it turns out I didn't understand "package-data" correctly before. In "package-data", we declare files to be included into the source distribution, and after that, with include-package-data = true we include them into the wheels.

@janosh Would appreciate it if you could give me a hand on this :)

@DanielYang59 DanielYang59 marked this pull request as ready for review September 7, 2024 04:39
@DanielYang59 DanielYang59 marked this pull request as draft September 7, 2024 05:22
@janosh
Copy link
Member

janosh commented Sep 7, 2024

@janosh Would appreciate it if you could give me a hand on this :)

happy to help. what's the problem exactly? that some cython files aren't included in the wheel?

@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Sep 7, 2024

Thanks for your reply @janosh :)

I originally opened this PR because I thought declaring all files was a bit inefficient (also prone to error if someone add a new file but forgot to update pyproject.toml), and I would expect all files included inside the src to be packed (really?).

The main cause of my headache is: the sdist need to include *.pyx but not *.so (the latter should not exist with python -m build --sdist so not a big issue), but the wheels should include *.so but not *.pyx (not sure if this is possible because with include-package-data = true everything from sdist would be copied to the wheel?).

But I didn't manage to find a more elegant yet safe way to handle file inclusion/exclusion at this moment (I hope I'm wrong).

As of my knowledge, to declare package data:

  1. (Current method) Explicitly declare all non-.py/pyx files.
  2. Add a MANIFEST.in file to declare package data file, but I don't think it's offering much benefit over current method.
  3. Use the following to include everything inside the src directory, but this would also include *.c and *.pyx files inside the wheels.
[tool.setuptools.package-data]
 "*" = ["*", ]
  1. Use setuptools-scm to include everything tracked by Git, but this would include tests as well.

I sneaked a peek at Jinzhe's DeepMD-kit and it looks like he's using a custom backend altogether.

@janosh
Copy link
Member

janosh commented Sep 7, 2024

but this would also include *.c and *.pyx files inside the wheels.

could you explain what the downsides of this are? is it a problem/bad practice to have those files in a wheel? seems like it could help with debugging to have the source files around if compiled code is ever causing issues. and the files are small so wouldn't noticeably increase wheel size.

@DanielYang59 DanielYang59 force-pushed the package-data branch 2 times, most recently from 42dedd0 to 50aeb2c Compare September 18, 2024 14:49
prune tests/

# Exclude individual files
exclude .* \
Copy link
Contributor Author

@DanielYang59 DanielYang59 Sep 18, 2024

Choose a reason for hiding this comment

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

I would rather accidentally include some useless files than missing some necessary ones, so I should be really careful using wildcard for exclusion.

Is there any hidden file needed (at least no from current distributions)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants