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

Fix mypy pep561 #1088

Merged
merged 1 commit into from
May 12, 2023
Merged

Conversation

WenjingKangIntel
Copy link
Contributor

@WenjingKangIntel WenjingKangIntel commented May 11, 2023

Description

  • Provide a summary of the modification as well as the issue that has been resolved. List any dependencies that this modification necessitates.

With this PR, when running
python setup.py install, python setup.py install sdist or python setup.py sdist bdist_wheel, py.typed will be included in the build package.
Snipaste_2023-05-11_18-38-04

Changes

  • Bug fix (non-breaking change which fixes an issue)
  • Refactor (non-breaking change which refactors the code base)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist

  • My code follows the pre-commit style and check guidelines of this project.
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing tests pass locally with my changes
  • I have added a summary of my changes to the CHANGELOG (not for minor changes, docs and tests).

Signed-off-by: Kang Wenjing <wenjing.kang@intel.com>
@WenjingKangIntel
Copy link
Contributor Author

WenjingKangIntel commented May 11, 2023

@ashwinvaidya17 Could you please help double check this? Thanks!

@ashwinvaidya17
Copy link
Collaborator

Do we really need sdist and bdist_wheel in the package?

@WenjingKangIntel
Copy link
Contributor Author

Do we really need sdist and bdist_wheel in the package?

Without this pr, even pure python setup.py install will not include py.typed in the package. FYI.

@WenjingKangIntel
Copy link
Contributor Author

WenjingKangIntel commented May 11, 2023

From this link:
https://setuptools.pypa.io/en/latest/userguide/datafiles.html#summary
include_package_data
Accept all data files and directories matched by MANIFEST.in or added by a plugin.

include_package_data needs to explicitly set to True because we are using MANIFEST.in to match files.

@ashwinvaidya17 ashwinvaidya17 self-requested a review May 11, 2023 11:46
@@ -107,6 +107,7 @@ def get_required_packages(requirement_files: list[str]) -> list[str]:
packages=find_packages(where="src", include=["anomalib", "anomalib.*"]),
install_requires=INSTALL_REQUIRES,
extras_require=EXTRAS_REQUIRE,
include_package_data=True,
package_data={"": ["config.yaml"]},
Copy link
Collaborator

Choose a reason for hiding this comment

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

From what I tested
package_data={"": ["config.yaml"], "anomalib": ["py.typed"]},
should be sufficient

Copy link
Contributor Author

@WenjingKangIntel WenjingKangIntel May 11, 2023

Choose a reason for hiding this comment

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

Yes, I've tested it as well. It worked. But if we are going to use MANIFEST.in, we need to use the include_package_data=True.
If we are going to use package_data={"": ["config.yaml"], "anomalib": ["py.typed"]},, then we need to remove py.typed from MANIFEST.in, because they have duplicated functionality, which is to include the same file.

To sum up, if we do not use include_package_data=True, MANIFEST.in will not take effect.

@samet-akcay samet-akcay merged commit 2473b5e into openvinotoolkit:main May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task]: Make anomalib PEP 561 compliant for mypy
3 participants