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

Change release procedure to use only pull requests #5250

Merged

Conversation

albertvillanova
Copy link
Member

@albertvillanova albertvillanova commented Nov 16, 2022

This PR changes the release procedure so that:

  • it only make changes to main branch via pull requests
  • it is no longer necessary to directly commit/push to main branch

Close #5251.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Nov 16, 2022

The documentation is not available anymore as the PR was closed or merged.

Copy link
Member

@lhoestq lhoestq left a comment

Choose a reason for hiding this comment

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

cool thanks ! a few comments:

setup.py Outdated
```
git checkout main
git pull upstream main
git checkout -b release
Copy link
Member

Choose a reason for hiding this comment

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

(nit) in case a branch with this name already exists

Suggested change
git checkout -b release
git checkout -b release-VERSION

Copy link
Member Author

Choose a reason for hiding this comment

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

I made it this way to make it simpler: as remote branches are deleted after the PR is merged.

Copy link
Member Author

Choose a reason for hiding this comment

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

If a local branch with the same name exists, then previous command generated this error:

fatal: A branch named 'release' already exists.

Copy link
Member

Choose a reason for hiding this comment

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

oh ok !

Copy link
Member Author

Choose a reason for hiding this comment

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

I have changed the release branch name so that it triggers docs generation: 88c9c83

Context:

Copy link
Member

Choose a reason for hiding this comment

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

it's fine to not have it unique then

Copy link
Member

Choose a reason for hiding this comment

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

If we have it unique we won't need to delete the branch on a local clone of the repo though - the branch exists temporarily on upstream, but locally it's not deleted when the PR is merged

Copy link
Member Author

Choose a reason for hiding this comment

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

The unique naming for the dev version would be more complicated though: something like vX.X.X+1.dev0-dev-version?

Maybe it is easier to add a step before to delete the local branch dev-version? See cbd5874

Copy link
Member

Choose a reason for hiding this comment

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

Yea let's add a step to delete the local branch then

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. That step was already added: cbd5874

Comment on lines +77 to +78
- Choose a tag: Introduce the new VERSION as tag, that will be created when you publish the release
- Create new tag VERSION on publish
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if it will trigger the release-conda CI job - it's configured like

on:
  push:
    tags:
      - "[0-9]+.[0-9]+.[0-9]+*"

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@lhoestq maybe we could align the triggering of release-conda to be the same as the one for generating the docs? That is: push to branch v*-release

Or maybe using release triggering instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

EDIT:
Indeed, conda-release was not triggered for 2.7.0 release...
Indeed it was triggered: https://github.com/huggingface/datasets/actions/runs/3478362090/jobs/5815637910

Any idea why, @lhoestq?

Copy link
Member

@lhoestq lhoestq Nov 17, 2022

Choose a reason for hiding this comment

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

We maintain the huggingface channel but not the conda forge one, which is maintained by the community

EDIT: it's now available on conda forge btw ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

OK!

Copy link
Member Author

Choose a reason for hiding this comment

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

I have reverted: 8694901

Copy link
Member

Choose a reason for hiding this comment

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

Push tag for both sounds good :)

setup.py Outdated
```
git add -u
git commit -m "Release: VERSION"
git push upstream release
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
git push upstream release
git push upstream release-VERSION

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

@albertvillanova
Copy link
Member Author

albertvillanova commented Nov 17, 2022

Little recap:

  • The release-conda GH action was properly triggered by push-tag event: therefore I guess this event is also created when we publish a release and create a tag within it (as it is the case in the new procedure)
  • The generate-documentation GH action will be triggered by the push-to-branch event if we align the name of the release branch with the expected regex v*-release
    • The naming has been aligned in the new procedure
    • Question: why do we have different triggering events for generate-doc and release-conda? Maybe we could set the same for both: either push-tag (when publishing the release), or push-to-branch
      • I think it will be better to use the push-tag event because in the new release procedure this happens later (when we publish the release), once we have already tested that everything works using the test-PyPI; on the contrary, the push-to-branch event happens before, even before opening the release PR: we could see afterwards that there is an issue, and cancel the Pull Request, but the docs and conda-package will already be published.
  • For the naming of the dev-version branch/PR, instead of having a complicated version naming, I'm proposing:

@albertvillanova
Copy link
Member Author

albertvillanova commented Nov 17, 2022

Just one question to be addressed: why do we have different triggering events for generate-doc and release-conda? Maybe we could set the same for both: either push-tag (when publishing the release), or push-to-branch

I think it will be better to use the push-tag event because in the new release procedure this happens later (when we publish the release), once we have already tested that everything works using the test-PyPI; on the contrary, the push-to-branch event happens before, even before opening the release PR: we could see afterwards that there is an issue, and cancel the Pull Request, but the docs and conda-package will already be published.

We could even use the release-published event instead: 8694901

Copy link
Member

@lhoestq lhoestq left a comment

Choose a reason for hiding this comment

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

Looks all good to me ! Thanks for improving the release procedure :)

Comment on lines +77 to +78
- Choose a tag: Introduce the new VERSION as tag, that will be created when you publish the release
- Create new tag VERSION on publish
Copy link
Member

Choose a reason for hiding this comment

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

Push tag for both sounds good :)

@albertvillanova
Copy link
Member Author

albertvillanova commented Nov 22, 2022

@lhoestq now that we have push-tag event for both build_documentation and release-conda, we have no constraint on the naming of the release branch:

@albertvillanova albertvillanova merged commit 703b843 into huggingface:main Nov 22, 2022
@albertvillanova albertvillanova deleted the change-release-procedure branch November 22, 2022 16:27
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.

Docs are not generated after latest release
3 participants