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

[FR] Include type information by default (*.pyi, py.typed) - PEP 561 #3136

Closed
1 task done
cdce8p opened this issue Feb 26, 2022 · 8 comments · Fixed by #4021
Closed
1 task done

[FR] Include type information by default (*.pyi, py.typed) - PEP 561 #3136

cdce8p opened this issue Feb 26, 2022 · 8 comments · Fixed by #4021

Comments

@cdce8p
Copy link
Contributor

cdce8p commented Feb 26, 2022

What's the problem this feature will solve?

PEP 561 specifies how type information should be included in a distributions. Namely package authors may choose to add py.typed files to fully typed packages and / or include .pyi stub files. By default, those files won't be included in the sdist / wheel distribution packages.

To add them, a package author needs to set include_package_data = True and include them via the MANIFEST.in or with options.package_data (eg. with * = py.typed, *.pyi). Although there are some great guides out there explaining it, I believe we could make this much simpler.

https://blog.whtsky.me/tech/2021/dont-forget-py.typed-for-your-typed-python-package/
https://jugmac00.github.io/blog/bite-my-shiny-type-annotated-library/

Describe the solution you'd like

Add a new option include_typing_files or include_typing_information as a shorthand for options.package_data-> * = py.typed, *.pyi.

How should it be used?
Authors wanting to include typing files (py.typed / stub files) should set both include_package_data = True and include_typing_files = True.

How would it work with other options to include package data files?
Conceptually this option would add the * = py.typed, *.pyi pattern to options.package_data. This also means any file matched by exclude_package_data should still be excluded.

Alternative Solutions

Don't require include_package_data = True. Similar to license files, one could argue if an author sets include_typing_files = True, they already expressed what they want to do. No need to require an additional option.

Include py.typed and *.pyi files by default.

Additional context

If others agree that this is something worth adding, I could work on a PR for it.

Code of Conduct

  • I agree to follow the PSF Code of Conduct
@cdce8p cdce8p added enhancement Needs Triage Issues that need to be evaluated for severity and status. labels Feb 26, 2022
@abravalheri
Copy link
Contributor

abravalheri commented Mar 23, 2022

Hi @cdce8p thank you very much for bringing this issue for discussion.

My personal opinion (which might not be shared by the setuptools maintainers) is that it is very unlikely that a new configuration option will be added. There is already a lot of options and people seem to have a hard time understanding/dealing with them.

Something that is more likely to happen would besetuptools starting to includ *.pyi files by default. Would that work for you?

One way to achieve that would be overriding the setuptools.commands.sdist.add_default method (or specifically _add_defaults_python) to add the type stubs on top of what is done in distutils.command.sdist:

def add_defaults(self):
"""Add all the default files to self.filelist:
- README or README.txt
- setup.py
- test/test*.py
- all pure Python modules mentioned in setup script
- all files pointed by package_data (build_py)
- all files defined in data_files.
- all files defined as scripts.
- all C sources listed as part of extensions or C libraries
in the setup script (doesn't catch C headers!)
Warns if (README or README.txt) or setup.py are missing; everything
else is optional.
"""
self._add_defaults_standards()
self._add_defaults_optional()
self._add_defaults_python()
self._add_defaults_data_files()
self._add_defaults_ext()
self._add_defaults_c_libs()
self._add_defaults_scripts()
@staticmethod
def _cs_path_exists(fspath):
"""
Case-sensitive path existence check
>>> sdist._cs_path_exists(__file__)
True
>>> sdist._cs_path_exists(__file__.upper())
False
"""
if not os.path.exists(fspath):
return False
# make absolute so we always have a directory
abspath = os.path.abspath(fspath)
directory, filename = os.path.split(abspath)
return filename in os.listdir(directory)
def _add_defaults_standards(self):
standards = [self.READMES, self.distribution.script_name]
for fn in standards:
if isinstance(fn, tuple):
alts = fn
got_it = False
for fn in alts:
if self._cs_path_exists(fn):
got_it = True
self.filelist.append(fn)
break
if not got_it:
self.warn("standard file not found: should have one of " +
', '.join(alts))
else:
if self._cs_path_exists(fn):
self.filelist.append(fn)
else:
self.warn("standard file '%s' not found" % fn)
def _add_defaults_optional(self):
optional = ['test/test*.py', 'setup.cfg']
for pattern in optional:
files = filter(os.path.isfile, glob(pattern))
self.filelist.extend(files)
def _add_defaults_python(self):
# build_py is used to get:
# - python modules
# - files defined in package_data
build_py = self.get_finalized_command('build_py')
# getting python files
if self.distribution.has_pure_modules():
self.filelist.extend(build_py.get_source_files())
# getting package_data files
# (computed in build_py.data_files by build_py.finalize_options)
for pkg, src_dir, build_dir, filenames in build_py.data_files:
for filename in filenames:
self.filelist.append(os.path.join(src_dir, filename))
def _add_defaults_data_files(self):
# getting distribution.data_files
if self.distribution.has_data_files():
for item in self.distribution.data_files:
if isinstance(item, str):
# plain file
item = convert_path(item)
if os.path.isfile(item):
self.filelist.append(item)
else:
# a (dirname, filenames) tuple
dirname, filenames = item
for f in filenames:
f = convert_path(f)
if os.path.isfile(f):
self.filelist.append(f)
def _add_defaults_ext(self):
if self.distribution.has_ext_modules():
build_ext = self.get_finalized_command('build_ext')
self.filelist.extend(build_ext.get_source_files())
def _add_defaults_c_libs(self):
if self.distribution.has_c_libraries():
build_clib = self.get_finalized_command('build_clib')
self.filelist.extend(build_clib.get_source_files())
def _add_defaults_scripts(self):
if self.distribution.has_scripts():
build_scripts = self.get_finalized_command('build_scripts')
self.filelist.extend(build_scripts.get_source_files())

Please also note that there is a related pending issue in distutils right now that would also improve the user experience when working with type stubs: pypa/distutils#108

@cdce8p
Copy link
Contributor Author

cdce8p commented Mar 23, 2022

Thanks for your comment @abravalheri!

Something that is more likely to happen would be setuptools starting to includ *.pyi files by default. Would that work for you?

Personally I'm more interested in py.typed tbh but including both types (*.pyi and py.typed) by default would certainly work. poetry and flit both do that although only because they just include all files within package directories.

@abravalheri
Copy link
Contributor

Personally I'm more interested in py.typed tbh but including both types (*.pyi and py.typed)

Oh yeah, I forgot about py.typed. I think including both by default would be ideal. Thanks for reminding me.

Would you be interested in working in a PR? There is no rush for that, but any help would be more than welcome!

@abravalheri abravalheri added help wanted and removed Needs Triage Issues that need to be evaluated for severity and status. labels Mar 23, 2022
@abravalheri abravalheri changed the title [FR] Add easy option to include type information - PEP 561 [FR] Include type information by default (*.pyi, py.typed) - PEP 561 Mar 23, 2022
@cdce8p
Copy link
Contributor Author

cdce8p commented Mar 23, 2022

Would you be interested in working in a PR? There is no rush for that, but any help would be more than welcome!

At the moment, I don't have the bandwidth for it unfortunately. If someone else would like to pick this up, that would be awesome.

@szabolcsdombi
Copy link

I am currently facing an issue distributing *.pyi files along my extensions.
For multiple simple projects I only have ext_modules= or py_modules= set. (no package folder needed for a one file project)

This seemed to work in the past, but I recently figured out that on some platforms the .pyi file is missing.

setup(
  name='mymodule',
  ext_modules=[Extension(name='mymodule', ...)],
  data_files=[('.', ['mymodule.pyi'])],
)

This code was indeed from "some great guides out there explaining it", but is broken as the '.' is not a valid package.
The source and wheel distributions will contain the data file but when installed the data file will be missing.

The expected installed version of the project should countain two files in the site-packages folder:

  • mymodule.___.pyd or mymodule.___.so
  • mymodule.pyi

Here I have a project with a similar setup.


I am interested in making the changes necessary.

One way to achieve that would be overriding the setuptools.commands.sdist.add_default method (or specifically _add_defaults_python) to add the type stubs on top of what is done in distutils.command.sdist:

Is this still the way to go?

@szabolcsdombi
Copy link

Here is a proof for no actual call is made to setuptools/setuptools/_distutils/command/sdist.py:_add_defaults_python()

@Danie-1
Copy link
Contributor

Danie-1 commented Aug 17, 2023

Based on the advice above, I decided to try modifying

def get_source_files(self):
return [module[-1] for module in self.find_all_modules()]

to this

def get_source_files(self):
    py_files = [module[-1] for module in self.find_all_modules()]

    possible_stub_files = set(os.path.splitext(f)[0] + ".pyi" for f in py_files)
    stub_files = [f for f in possible_stub_files if os.path.isfile(f)]

    possible_py_typed_files = set(os.path.dirname(f) + "/py.typed" for f in py_files)
    py_typed_files = [f for f in possible_py_typed_files if os.path.isfile(f)]

    return py_files + stub_files + py_typed_files

I wasn't sure how to test this, but I tested it in the following way and it was successful:

  • I created the following directory structure (note that pyproject.toml just contained project name, version, requires_python and the build-system info, so not the usual "py.typed" stuff):
    pyproject.toml
    setup.py
    src/module_name/__init__.py
    src/module_name/py.typed
    src/module_name/file.py
    src/module_name/file.pyi
    
  • I created a venv and installed with pip install -e this git repository with my changes shown above. Then I install mypy and wheel as well.
  • Then I ran python setup.py bdist_wheel and ran pip install on the wheel in dist.
  • I created test_file.py in the root repository and just put import module_name and then ran mypy on it and got no warnings.
    When I test the exact same method but without my code changes, mypy reports errors. This test also works if src/module_name is replaced by module_name, and for the mypy test stage you need to put test_file.py in a new folder and run mypy from there so that it detects the installation rather than the source files.

I tried creating a test for this by modifying

def test_add_defaults(self):

by adding lines to create some py.typed and .pyi files and adding more entries to expected but when I run tox I don't think this test runs. (Also when I run tox I always get one failing test that seems to be to do with an error with pip installing something to do with @git+https://...)

@abravalheri
Copy link
Contributor

Hi @Danie-1, would you like to provide a PR for that? We can iterate over it and iron out the issues...

Just one thing, for an eventual PR, it would be important to modify (setuptools/command/build_py.py)(https://github.com/pypa/setuptools/blob/main/setuptools/command/build_py.py] instead of _distutils (you can use inheritance to overwrite methods).

The reason why we should avoid modifying files under _distutils is that this directory is maintained externally in https://github.com/pypa/distutils.

Also when I run tox I always get one failing test that seems to be to do with an error with pip installing something to do with @git+https://...

Yeah, sorry this is unfortunate. You can try running tox -- -p no:perf to disable the performance tests that try to git clone from github.

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

Successfully merging a pull request may close this issue.

4 participants