DOC clarifications on the release process #15759
Conversation
|
@jnothman anything I have as mistakes here? @NicolasHug WDYT? I'm happy to clarify wherever's not clear. |
doc/developers/maintainer.rst
Outdated
| Since any commits to an existing branch (e.g. 0.999.X) will automatically | ||
| update the web site documentation, it is best to develop a release with a pull | ||
| request in which 0.999.X is the base. It also allows you to keep track of any | ||
| tasks towards release with a TO DO list. |
There was a problem hiding this comment.
I don't understand this, what documentation is updated exactly, and why is it a bad thing?
|
As external observer I would like to read here something explaining the different types of release (like the python one for example).... obviously this can wait another PR. |
doc/developers/maintainer.rst
Outdated
| ...................... | ||
|
|
||
| Since any commits to an existing branch (e.g. 0.999.X) will automatically | ||
| update the web site documentation, it is best to develop a release with a pull | ||
| request in which 0.999.X is the base. It also allows you to keep track of any |
There was a problem hiding this comment.
Not sure I understand what "0.999.X is the base" means. does it mean that the PR tries to merge things into the 0.999.X branch? Is there any other way to do it anyway?
There was a problem hiding this comment.
For me the two git commands a few lines later explain what it is. I also didn't understand what it meant from the text.
doc/developers/maintainer.rst
Outdated
| Since any commits to an existing branch (e.g. 0.999.X) will automatically | ||
| update the web site documentation, it is best to develop a release with a pull |
There was a problem hiding this comment.
Is it true that any branch to the repo has an online documentation? I thought we'd have a way to curate which branches we want to have the docs for, like with ReadTheDocs
There was a problem hiding this comment.
AFAIK any 0.XX.X branch has a documentation online. Any change to the 0.21.X branch for example, will be there almost immediately. And to add a new release, you just create the branch, and it'll automatically be there.
There was a problem hiding this comment.
But will it really automatically appear in https://scikit-learn.org/dev/versions.html?
There was a problem hiding this comment.
Yes, I think it does, @NicolasHug, but it's not listed there as stable until the symlink is updated
That's a good point. I also read that one while setting the tags. I left a link to it. |
There was a problem hiding this comment.
Trying to summarize the paragraph that I don't understand:
First, create a branch (to release e.g. 0.999.3):
$ # assuming master and upstream/master are the same
$ git checkout -b release-0.999.3 master
Note that pushing the (empty) branch will
automatically upload its documentation, which currently
corresponds to that of master, so it's not an accurate representation
of what the release will actually be.
Then create a PR with all the desired changes:
$ git rebase -i upstream/0.999.2
Ideally, the PR should be merged quickly, to minimize the
gap between the time when the website points to
a release, and the release
being available to users.
doc/developers/maintainer.rst
Outdated
| $ git checkout -b release-0.999.3 master | ||
| $ git rebase -i 0.999.X | ||
| $ git rebase -i upstream/0.999.X |
There was a problem hiding this comment.
should X be 2 in this case? Can it be something else?
There was a problem hiding this comment.
X is a literal here. It's the branch name
There was a problem hiding this comment.
what branch name? the branch we're creating here is named release-0.999.3
doc/developers/maintainer.rst
Outdated
| Since any commits to an existing branch (e.g. 0.999.X) will automatically | ||
| update the web site documentation, it is best to develop a release with a pull |
There was a problem hiding this comment.
But will it really automatically appear in https://scikit-learn.org/dev/versions.html?
doc/developers/maintainer.rst
Outdated
| available to users. Having all the changes for a release in one PR minimizes | ||
| the gap between the time when the website points to a release, and the release | ||
| being available to users. |
There was a problem hiding this comment.
I don't understand why only having one PR minimizes the delta? Since the doc is created as soon as the branch is pushed, whether you're doing multiple PRs or just one is just a matter on how fast all of them get merged right?
There was a problem hiding this comment.
yes, having one PR just makes it clearer what the difference between two releases are.
There was a problem hiding this comment.
So the current description should be updated. Right now it says have only one PR helps minimizing the time gap, which isn't true
doc/developers/maintainer.rst
Outdated
|
|
||
| - *maintainer* role on ``scikit-learn`` projects on ``pypi.org`` and | ||
| ``test.pypi.org``, separately. | ||
| - become a member of the *scikit-learn* team on conda-forge. |
There was a problem hiding this comment.
Note that this requires editing recipe/meta.yaml
doc/developers/maintainer.rst
Outdated
| Since any commits to an existing branch (e.g. 0.999.X) will automatically | ||
| update the web site documentation, it is best to develop a release with a pull |
There was a problem hiding this comment.
Yes, I think it does, @NicolasHug, but it's not listed there as stable until the symlink is updated
doc/developers/maintainer.rst
Outdated
| $ git checkout -b release-0.999.3 master | ||
| $ git rebase -i 0.999.X | ||
| $ git rebase -i upstream/0.999.X |
There was a problem hiding this comment.
X is a literal here. It's the branch name
doc/developers/maintainer.rst
Outdated
| $ sed -i "s/latestStable = '.*/latestStable = '0.999';" versionwarning.js | ||
| $ git commit -m "Update stable to point to 0.999" stable | ||
| $ sed -i "s/latestStable = '.*/latestStable = '0.999';/g" versionwarning.js | ||
| $ git commit -am "Update stable to point to 0.999" |
There was a problem hiding this comment.
I don't get why you prefer this to explicitly only committing stable
There was a problem hiding this comment.
the stable used to be the only thing changing. Now versionwarning.js also changes in this step.
There was a problem hiding this comment.
Added git add stable/ versionwarning on the line above to be explicit.
doc/developers/maintainer.rst
Outdated
| @@ -119,8 +182,8 @@ Making a release | |||
| $ git checkout master | |||
| $ rm stable | |||
| $ ln -s 0.999 stable | |||
| $ sed -i "s/latestStable = '.*/latestStable = '0.999';" versionwarning.js | |||
| $ git commit -m "Update stable to point to 0.999" stable | |||
| $ sed -i "s/latestStable = '.*/latestStable = '0.999';/g" versionwarning.js | |||
There was a problem hiding this comment.
yeah, my sed complained about an unfinished s/ directive or something w/o it.
There was a problem hiding this comment.
You're right. It needs the / but not the g
|
In the first paragraph, I added how the branches are named, does that help clarify the confusing paragraph @NicolasHug ? |
There was a problem hiding this comment.
Made a few comments, but sorry I still find the paragraph confusing.
Maybe comment on what isn't correct about my suggestion in #15759 (review) (this is my current understanding of things)
doc/developers/maintainer.rst
Outdated
| maintainers to review before the release. Note that all development for the | ||
| minor releases and the major release of for instance 0.99, happens under the | ||
| branch called ``0.99.X``, where ``X`` is a literal, and independent of minor | ||
| bug fix releases. Each release candidate, major, or minor release is a tag |
There was a problem hiding this comment.
I don't understand this sentence, especially since it is said above that changes should be done to master whenever possible
There was a problem hiding this comment.
The development happens on that branch, through PRs which are merged into that branch.
doc/developers/maintainer.rst
Outdated
| happen in master to avoid asynchrony. To select commits from master for use in | ||
| the bug fix (version 0.999.3), you can use:: | ||
| Most development of the release, and its documentation, should happen in master | ||
| to avoid asynchrony. First, create a branch, **on your own fork** (to release |
There was a problem hiding this comment.
This whole first paragraph and the first sentence of the second one feel redundant with the rest
There was a problem hiding this comment.
Not sure how to fix that.
There was a problem hiding this comment.
I have suggested an alternative paragraph before
doc/developers/maintainer.rst
Outdated
| branch, for the other maintainers to review before the release. Note that all | ||
| development for the minor releases and the major release of for instance 0.99, | ||
| happens under the branch called ``0.99.X``, where ``X`` is a literal, and | ||
| independent of minor bug fix releases. Each release candidate, major, or minor |
There was a problem hiding this comment.
I still think this is ambiguous given the previous sentence. Please clarify what "development happens" means or change the phrasing?
|
I tried to address them. If they're still not clear, feel free to apply changes directly. Don't really feel like working on this PR for much longer :D |
There was a problem hiding this comment.
@adrinjalali even if I'm not a core dev may I approve? I feel as never ending reviews are contraindicated for core-devs too, not only for beginners... :)
The best review ever for this PR will be the next release. WDYT?
There was a problem hiding this comment.
Thanks @cmarmo ! Yes let's merge this, it been a quite long and detailed discussed for a documentation change that only impacts the release manager (who will likely be able to detect if anything is amiss while going through the instructions).
I think most comments are addressed, waiting 1 day to see if there are any objections.
|
let's not merge yet. Last time I looked it wasn't clear to me, and I haven't had to time to check again. There's no rush anyway |
|
OK I added a few minor fixed, IMO it's good to go.
It's been 1.5 months, 5 reviewers and 81 comments, for something that was expected to be just Adrin documenting his experience as release manager. Never ending reviews are frustrating both for PR authors and reviewers. You were mostly fine with it in December i think #15759 (comment) ? I would rather we merged it and made follow up clarifications if needed. In any case I see this as a rough guideline for the release manager, and would expect them to have an overall picture of what needs to be done, be sure they understand each of those commands before executing them and asking questions/making clarification PRs if there is any doubt. |
I was only referring to Adrin's modifications to my suggestion further up, not to the PR as a whole. I don't think there's any urgency here, this is not even a user-facing doc. I'll try to review and push my own stuff soon (by the end of next week) |
There was a problem hiding this comment.
I have pushed some changes. I still have a few questions left, please take a look when you can @adrinjalali
You could also merge and have your changes in a separate PR :P which would make it easier for me and others to review. I'm kinda lost now :D Have you removed the part where I talked about why the release should ideally happen very quickly after the PR is merged? I think that's an important point if it's missing now. |
Yes I removed it because I could not make sense out of it. The cause to effect relationships were broken somewhere. Here is what it was In particular, the sentence "Note that pushing the (empty) branch to the main repo will automatically upload its documentation" is confusing because right above the branch is created on the fork. So it's not clear what branch this is referring to.
Feel free to merge, I pushed because you suggested to before |
|
Thank you all for the commits and comments. Merging this one, happy to have further improvements on it. |
* some extra comments * reorganize steps * more info * more clarification * further instructions * be cautious with tagging * add pypi note * git log copy * upstream * more explanation * fix typo * clarify * typo * further clarifications, address comments * add link to pep101 * explain branch names * apply comments * cover more comments * clarify branching, remove Dash ref * Update maintainer.rst * address some of Roman's comments * address some of Nocolas's suggestions * trying to address comments * CLN Adds reference to conda-forge recipe/meta.yml * DOC Explicitly push the tag by name * DOC Explicitly git add folders before commiting * fixed ref and removed redundant sentence (I think) * Added note about making sure derecations are handled * fixed title headers, this file is not just about releasing * removed redundant paragraph * major changes * minor comments Co-authored-by: Hanmin Qin <qinhanmin2005@sina.com> Co-authored-by: Thomas J Fan <thomasjpfan@gmail.com> Co-authored-by: Roman Yurchak <rth.yurchak@gmail.com> Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>
* some extra comments * reorganize steps * more info * more clarification * further instructions * be cautious with tagging * add pypi note * git log copy * upstream * more explanation * fix typo * clarify * typo * further clarifications, address comments * add link to pep101 * explain branch names * apply comments * cover more comments * clarify branching, remove Dash ref * Update maintainer.rst * address some of Roman's comments * address some of Nocolas's suggestions * trying to address comments * CLN Adds reference to conda-forge recipe/meta.yml * DOC Explicitly push the tag by name * DOC Explicitly git add folders before commiting * fixed ref and removed redundant sentence (I think) * Added note about making sure derecations are handled * fixed title headers, this file is not just about releasing * removed redundant paragraph * major changes * minor comments Co-authored-by: Hanmin Qin <qinhanmin2005@sina.com> Co-authored-by: Thomas J Fan <thomasjpfan@gmail.com> Co-authored-by: Roman Yurchak <rth.yurchak@gmail.com> Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>
adding a few clarifications to the release process.