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

Merged
merged 9 commits into from
Jun 6, 2024

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 <[email protected]>
@anmyachev anmyachev marked this pull request as ready for review May 14, 2024 10:14
.github/workflows/publish-to-pypi.yml Outdated Show resolved Hide resolved
.github/workflows/publish-to-pypi.yml Outdated Show resolved Hide resolved
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

anmyachev and others added 3 commits May 29, 2024 19:51
Comment on lines +25 to +26
run: git checkout $(git describe --tags "$(git rev-list --tags --max-count=1)")
if: github.event_name == 'push'
Copy link
Contributor

Choose a reason for hiding this comment

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

In my suggestion I swapped the order of these 😢

Suggested change
run: git checkout $(git describe --tags "$(git rev-list --tags --max-count=1)")
if: github.event_name == 'push'
if: github.event_name == 'push'
run: git checkout $(git describe --tags "$(git rev-list --tags --max-count=1)")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In my suggestion I swapped the order of these 😢

Actually, I've adjusted your suggestion a bit to make the style similar to what is used in ci.yml :) Does it make sense?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, is this valid too? If so I'm okay to merge 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.

Oh, is this valid too?

yes

@anmyachev anmyachev merged commit dea0003 into modin-project:main Jun 6, 2024
34 checks passed
@anmyachev anmyachev deleted the auto-publish-wheel branch June 6, 2024 16:27
@YarShev
Copy link
Collaborator

YarShev commented Jun 10, 2024

@anmyachev, we should also have updated the release docs.

anmyachev added a commit that referenced this pull request Jun 10, 2024
Signed-off-by: Anatoly Myachev <[email protected]>
Co-authored-by: Devin Petersohn <[email protected]>
anmyachev added a commit that referenced this pull request Jun 10, 2024
Signed-off-by: Anatoly Myachev <[email protected]>
Co-authored-by: Devin Petersohn <[email protected]>
anmyachev added a commit that referenced this pull request Jun 10, 2024
Signed-off-by: Anatoly Myachev <[email protected]>
Co-authored-by: Devin Petersohn <[email protected]>
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