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.

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

change: Update package metadata #3529

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

change: Update package metadata #3529

wants to merge 12 commits into from
+257 −200

Conversation

ofek
Copy link

@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
Member

@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
Member

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

@knikure knikure added this to To do in PR Queue via automation Dec 13, 2022
@ofek
Copy link
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
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
Copy link
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
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

@ofek ofek temporarily deployed to manual-approval June 21, 2024 18:29 — with GitHub Actions Inactive
@ofek ofek temporarily deployed to manual-approval June 22, 2024 01:47 — with GitHub Actions Inactive
@ofek ofek temporarily deployed to manual-approval June 22, 2024 17:30 — with GitHub Actions Inactive
@ofek
Copy link
Author

ofek commented Jun 24, 2024

This appears to be passing now however there are stylistic errors corresponding to files that were not changed. I was going to merge the latest master but you've already done that so I'm not sure what to do.

@ofek
Copy link
Author

ofek commented Jun 24, 2024

The good news is that the proposed style change is not erroneous! The bad news is that somehow it's not doing the right thing on master (and other PRs).

@benieric
Copy link
Contributor

Yeah lol, not sure why the style change issue is not being shown in master branch

@benieric
Copy link
Contributor

Looks like the proposed style changes are correct in this PR, can we do those and include with this change? For black should be able to just do tox -e black-format

@ofek ofek temporarily deployed to manual-approval June 24, 2024 20:09 — with GitHub Actions Inactive
@ofek
Copy link
Author

ofek commented Jun 24, 2024

Done!

@ofek
Copy link
Author

ofek commented Jun 25, 2024 •
edited
Loading

Since Python 2 is no longer supported by this project you can remove the requirement to add a from __future__ import absolute_import at the top of every file. In any case, I fixed that error and I think if you rerun CI everything should pass now!

benieric
benieric reviewed Jun 26, 2024
View reviewed changes
tox.ini Outdated Show resolved Hide resolved
@ofek ofek temporarily deployed to manual-approval June 26, 2024 19:57 — with GitHub Actions Inactive
PR Queue automation moved this from To do to Reviewer Approved Jun 26, 2024
@ofek
Copy link
Author

ofek commented Jun 26, 2024

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

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

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

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

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

@benieric benieric benieric approved these changes

Assignees

@mufaddal-rohawala mufaddal-rohawala

@benieric benieric

Labels
None yet
Projects
PR Queue
Reviewer Approved
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@ofek @navinsoni @sagemaker-bot @mufaddal-rohawala @jmahlik @knikure @benieric