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

FEAT-#7265: Automatic publication of Modin wheel to PyPI #7262

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

anmyachev
Copy link
Collaborator

@anmyachev anmyachev commented May 14, 2024

What do these changes do?

  • first commit message and PR title follow format outlined here

    NOTE: If you edit the PR title to match this format, you need to add another commit (even if it's empty) or amend your last commit for the CI job that checks the PR title to pick up the new PR title.

  • passes flake8 modin/ asv_bench/benchmarks scripts/doc_checker.py
  • passes black --check modin/ asv_bench/benchmarks scripts/doc_checker.py
  • signed commit with git commit -s
  • Resolves Automatic publication of Modin wheel to PyPI #7265
  • tests added and passing
  • module layout described at docs/development/architecture.rst is up-to-date

@anmyachev anmyachev changed the title FEAT-#0000: Automatic publication of Madin wheel to PyPI FEAT-#0000: Automatic publication of Modin wheel to PyPI May 14, 2024
@anmyachev anmyachev changed the title FEAT-#0000: Automatic publication of Modin wheel to PyPI FEAT-#7265: Automatic publication of Modin wheel to PyPI May 14, 2024
Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>
Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>
@anmyachev anmyachev marked this pull request as ready for review May 14, 2024 10:14
Co-authored-by: Devin Petersohn <devin.petersohn@snowflake.com>
path: ./dist/

- name: Publish Modin wheel to PyPI
if: startsWith(github.ref, 'refs/tags')
Copy link
Collaborator

Choose a reason for hiding this comment

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

We likely want to run the entire job on a new tag. Let's lift this up.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 left a comment above for how to do this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will leave this if statement as additional safeguard against accidental changes if you don't mind.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I looked at how this was done in Pandas and Numpy, everywhere it is possible to launch through workflow_dispatch event or according to a schedule to make sure that everything works correctly. For such checks to work, we need to remove triggering only for tags.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It should work to add workflow_dispatch under on: if we want to also manually trigger it. Multiple options can be put in the on setting

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added events: workflow_dispatch and schedule.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest we only upload when the trigger is a push, rather than looking at the ref.

Suggested change
if: startsWith(github.ref, 'refs/tags')
if: github.event_name == "push"

.github/workflows/publish-to-pypi.yml Outdated Show resolved Hide resolved
path: ./dist/

- name: Publish Modin wheel to PyPI
if: startsWith(github.ref, 'refs/tags')
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 left a comment above for how to do this.

runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v4
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should checkout Modin with fetch_depth: 0, otherwise only a latest commit is fetched.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should have tagged wheels, otherwise we might get untagged ones.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is possible the latest commit is not the tagged commit, so I think @YarShev is correct. Additionally, we may want to check out the latest tag to ensure we only upload tagged releases.

It would be good to also have a check that ensures that the release name is correct, so it doesn't upload a bad release.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will it be better to use fetch-tags: option?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes that would be good, but we should also git checkout the latest tag to ensure we build the correct release. It seems like we still need fetch-depth:0 for fetch-tags to work properly: actions/checkout#1471

Co-authored-by: Devin Petersohn <devin-petersohn@users.noreply.github.com>
anmyachev and others added 3 commits May 29, 2024 19:51
Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>
Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>
on:
schedule:
- cron: "42 0 * * WED"
push:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this got deleted somewhere. We need it so that this triggers for every new tag.

Suggested change
push:
push:
tags:
- '*'

path: ./dist/

- name: Publish Modin wheel to PyPI
if: startsWith(github.ref, 'refs/tags')
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest we only upload when the trigger is a push, rather than looking at the ref.

Suggested change
if: startsWith(github.ref, 'refs/tags')
if: github.event_name == "push"

with:
fetch-depth: 0
fetch-tags: true
- name: Set up Python
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to check out the latest tag because that might not be where head is (for point releases and if new commits are merged after the tag)

Suggested change
- name: Set up Python
- name: Checkout latest git tag
- if: github.event_name == "push"
git checkout $(git describe --tags "$(git rev-list --tags --max-count=1)")
- name: Set up Python

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.

Automatic publication of Modin wheel to PyPI
4 participants