Skip to content

change: Update package metadata #3529

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 18 commits into from
Jul 31, 2024
Merged

change: Update package metadata #3529

merged 18 commits into from
Jul 31, 2024

Conversation

ofek
Copy link
Contributor

@ofek ofek commented Dec 12, 2022 •
edited
Loading

Background

Hello there! The Python packaging ecosystem has standardized on the interface for build backends (PEP 517/PEP 660) and the format for metadata declaration (PEP 621/PEP 631). As a result, the execution of setup.py files is now deprecated.

So, I'm spending my free time updating important projects so that they are modernized and set an example for others 😄

Summary of changes

This implements PEP 621, obviating the need for setup.py, setup.cfg, and MANIFEST.in. The build backend hatchling (of which I am a maintainer in the PyPA) is now used as that is the default in the official Python packaging tutorial. Hatchling is available on all the major distribution channels such as Debian, Fedora, Arch Linux, conda-forge, Nixpkgs, Alpine Linux, FreeBSD/OpenBSD, Gentoo Linux, MacPorts, OpenEmbedded, Spack, MSYS2, etc.

Notes

  • The source distributions on PyPI are shipping /requirements/tox even though /tox.ini is not; let me know if you want me to remove the former or add the latter.
  • The project is no longer polluted with build artifacts such as a *.egg-info directory.
  • (See "Why Hatch?") The project now supports static analysis for external tools such as IDEs. With setuptools, you must provide additional configuration for editable installations which means that by default, for example, you would not get autocompletion in Visual Studio Code. This is marked as a legacy feature and may in fact be removed in future versions of setuptools.

Testing done

tox -e py39

Merge Checklist

General

  • I have read the CONTRIBUTING doc
  • I certify that the changes I am introducing will be backward compatible, and I have discussed concerns about this, if any, with the Python SDK team
  • I used the commit message format described in CONTRIBUTING
  • I have passed the region in to all S3 and STS clients that I've initialized as part of this change.
  • I have updated any necessary documentation, including READMEs and API docs (if appropriate)

Tests

  • I have added tests that prove my fix is effective or that my feature works (if appropriate)
  • I have added unit and/or integration tests as appropriate to ensure backward compatibility of the changes
  • I have checked that my tests are not configured for a specific region or account (if appropriate)
  • I have used unique_name_from_base to create resource names in integ tests (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ofek ofek requested a review from a team as a code owner December 12, 2022 06:44
@ofek ofek requested review from navinsoni and removed request for a team December 12, 2022 06:44
navinsoni
navinsoni reviewed Dec 12, 2022
View reviewed changes
Copy link
Contributor

@navinsoni navinsoni left a comment

Choose a reason for hiding this comment

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

/bot run all

@navinsoni
Copy link
Contributor

navinsoni commented Dec 12, 2022 •
edited
Loading

@ofek Thank you for contributing to sagemaker-python-sdk. We will have to update our backend to make sure other PRs work with this change. I will check with my team and prioritize it accordingly.

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: 8b97b85
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: 8b97b85
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-slow-tests
  • Commit ID: 8b97b85
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: 8b97b85
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: 8b97b85
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@ofek ofek force-pushed the modernize-metadata branch from e1d9f5f to eb43677 Compare December 15, 2022 21:59
@ofek
Copy link
Contributor Author

ofek commented Dec 15, 2022

@navinsoni I rebased; would you mind triggering the CI again?

mufaddal-rohawala
mufaddal-rohawala reviewed Dec 19, 2022
View reviewed changes
Copy link
Member

@mufaddal-rohawala mufaddal-rohawala left a comment

Choose a reason for hiding this comment

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

/bot run all

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: 3c51dfc
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: 3c51dfc
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: 3c51dfc
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: 3c51dfc
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-slow-tests
  • Commit ID: 3c51dfc
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@ofek
Copy link
Contributor Author

ofek commented Dec 20, 2022

Umm @mufaddal-rohawala did you intend to force push to the master branch? That broke everyone's local Git and open PR

@ofek ofek force-pushed the modernize-metadata branch from 29e2b1d to 4d952ff Compare December 20, 2022 01:03
@ofek
Copy link
Contributor Author

ofek commented Dec 20, 2022

I fixed my fork and branch, RIP to all of the others 😅

I'd highly recommend setting up branch protection to prevent this from happening again https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/about-protected-branches#allow-force-pushes

@ofek
Copy link
Contributor Author

ofek commented Jan 10, 2023

Would you mind triggering the CI again?

mufaddal-rohawala
mufaddal-rohawala reviewed Feb 13, 2023
View reviewed changes
Copy link
Member

@mufaddal-rohawala mufaddal-rohawala left a comment

Choose a reason for hiding this comment

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

/bot run all

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: 4d952ff
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-slow-tests
  • Commit ID: 4d952ff
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: 4d952ff
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@ofek ofek temporarily deployed to manual-approval June 26, 2024 19:57 — with GitHub Actions Inactive
benieric
benieric previously approved these changes Jun 26, 2024
View reviewed changes
@ofek
Copy link
Contributor Author

ofek commented Jun 26, 2024

Looks like a flaky test, might just need to rerun that particular failed job.

@ofek ofek temporarily deployed to manual-approval June 26, 2024 22:03 — with GitHub Actions Inactive
@benieric
Copy link
Contributor

Thanks for working on this @ofek , will get one more review from team member before we get this merged!

@ofek
Copy link
Contributor Author

ofek commented Jul 8, 2024

Hello again, I just wanted to follow-up to see how things are going!

@benieric
Copy link
Contributor

Sorry for the delay, I am reaching out to team member today to get a second review!

@benieric
Copy link
Contributor

@ofek Could you take a look at the conflicts please. @akrishna1995 has agreed to help review this one

@ofek ofek temporarily deployed to manual-approval July 16, 2024 21:26 — with GitHub Actions Inactive
@ofek
Copy link
Contributor Author

ofek commented Jul 16, 2024

Sure, just updated the branch!

@benieric
Copy link
Contributor

Thanks, sorry again for the churn

benieric
benieric reviewed Jul 19, 2024
View reviewed changes
pyproject.toml Outdated
dynamic = ["version", "optional-dependencies"]
description = "Open source library for training and deploying models on Amazon SageMaker."
readme = "README.rst"
license = "Apache-2.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just saw this but can you change this to be license = {file = "LICENSE.txt"} like mentioned here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is outdated actually the standard is being updated by PEP 639 which was always planned (allowing a string value). The license file is automatically picked up by Hatchling and included in the source distribution.

Copy link
Contributor

Choose a reason for hiding this comment

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

The only blocker I have with this one currently is that when I run python setup.py sdist locally I get an error like:

configuration error: `project.license` must be valid exactly by one definition (2 matches found):

    - keys:
        'file': {type: string}
      required: ['file']
    - keys:
        'text': {type: string}
      required: ['text']

DESCRIPTION:
    `Project license <https://peps.python.org/pep-0621/#license>`_.

GIVEN VALUE:
    "Apache-2.0"

OFFENDING RULE: 'oneOf'

Copy link
Contributor

Choose a reason for hiding this comment

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

Because PEP 639 is still in Draft can we do license = {file = "LICENSE.txt"} until then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! I removed the field instead because the license is automatically added to the source distribution.

@ofek ofek temporarily deployed to manual-approval July 27, 2024 20:22 — with GitHub Actions Inactive
@benieric benieric temporarily deployed to manual-approval July 30, 2024 22:42 — with GitHub Actions Inactive
@benieric benieric merged commit 330e47a into aws:master Jul 31, 2024
14 checks passed
@ofek ofek deleted the modernize-metadata branch July 31, 2024 18:10
@ofek
Copy link
Contributor Author

ofek commented Jul 31, 2024

Thanks for the perseverance here! I will start helping your organization migrate other projects in my free time 🙂

@benieric benieric mentioned this pull request Aug 21, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@navinsoni navinsoni navinsoni left review comments

@benieric benieric benieric approved these changes

@mufaddal-rohawala mufaddal-rohawala mufaddal-rohawala left review comments

Assignees

@mufaddal-rohawala mufaddal-rohawala

@benieric benieric

Labels
None yet
Projects
PR Queue
Status: Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ofek @navinsoni @sagemaker-bot @mufaddal-rohawala @jmahlik @knikure @benieric