Please avoid non-bugfix changes during the beta phase

Hey there Team,

please don’t merge non-bugfix changes to the 3.9 branch during the beta phase unless absolutely necessary. I noticed a few changes merged in the past few weeks which don’t belong in a beta release. My point isn’t to dissect those cases now, nor is it to split hair on which compromise is better. I reviewed the changes in question and asked around for advice, ultimately deciding I won’t be reverting them. Nevertheless, I would like to remind you of this part of the Developer’s Guide:

https://devguide.python.org/devcycle/#beta

We follow a release process which leaves time specifically for bugfix-only changes (starting with Beta 1 for a given version). We do this to minimize risk and stabilize a release towards the final “point oh” release. The risk is minimized that way because:

  • there is simply fewer incoming changes if we limit the set of acceptable changes;
  • both core developers and testers can focus on features which are already implemented (we are not a moving target); and most importantly
  • testers who ran tests with, say, Beta 2 can reasonably assume their program will work with the final release.

Be a team player, please, and respect the beta freeze. If you need an exception, that’s fine, it happens. I reviewed and accepted some during the 3.9.0 beta phase already. But in such a case please bring more people to the table so we can decide together. It doesn’t have to be me, I don’t aspire to be a gatekeeper, and I hope you see that I avoid using the revert hammer unless absolutely necessary. All I care about is that we produce releases of the highest quality and the community beta testing phase is a crucial component of that.

On code review

One last thing. There’s been some controversy around whether code review should be mandatory or not. This topic discusses the matter: Make code review mandatory?

Long story short, we cannot make code reviews mandatory because the team is too small, the codebase too big, and the volunteer nature of the project means we cannot expect any rate of contribution. In other words, Python would grind to a halt with mandatory review.

However, please note that those reservations talk about the regular development process. There are specific circumstances where we need more care. The Developer’s Guide specifically says review is indeed mandatory during the release candidate phase, see:

https://devguide.python.org/devcycle/#release-candidate-rc

By extension, I think it’s fair to expect at least some form of discussion on changes which fall outside of the acceptable set for the beta phase.

Cheers,
Ł

12 Likes

It’ll be up to the steering council to decide if we want this process, but it seems within reason for us to make merges require a review from another committer before merging into a branch in the beta phase.

3 Likes

Along the same lines, but maybe a bit less intrusive: have the requirement for any PR to a branch in beta phase that doesn’t have the type-documentation or type-bugfix label. Otherwise, I could see it blocking bugfix PRs on release manager approval when the only person available and knowledgeable in that area is the author. It could also add a significant delay for small doc changes, which can be merged without issue during the beta.

Of course, someone could add the type-bugfix label for an enhancement PR to circumvent the check, but I fully trust core devs to appropriately use the labels.

2 Likes

And this is ultimately the problem - all the problematic commits I’m aware of recently have been called bugfixes by the authors. It was only in later testing that we discovered something had broken, and found at the root that it was a change that shouldn’t have been made during beta.

So that trust has already been broken, which is why this post was written. We also need to guide/educate our team on what constitutes a “non-bugfix” change.

1 Like

It seems then that the primary issue is specifically a misunderstanding of what a “bugfix” strictly is for the purposes of changes that can be merged during the beta. To me at least, this differs significantly from trust being broken.

Fixing a regression issue from code that previously worked (not including usage of private or deprecated-removed APIs) seems like a clear bugfix that could be merged during the beta. But, it’s not entirely clear to me (and perhaps others) when it comes to some behavioral changes. For example, what if the documented specifications were not entirely aligned with the actual behavior of a member, and a code change was made to address it? What about a discovered edge case that causes undesirable behavior in production? Generally speaking, should those types of fixes be merged to a beta branch?

I think the main way to solve this issue is by providing a few generically applicable examples of changes that are and aren’t acceptable to merge during the beta. Otherwise, it will likely end up differing based on the core developer that reviews the PR, assuming the author waits for at least one approval prior to merging.

Realistically speaking, we can’t reasonably expect the release manager to be able to review every single code change that could be subjectively interpreted as a “bugfix” for the beta. Especially as CPython development continues to scale in contributors and core team size, this becomes increasingly unsustainable. So, I think a good balance would be laying out clear examples of bugfixes as described above, and then contacting the release manager for unclear cases outside of that.

At a glance, it seems like a good location for this would in the Development Cycle section of the devguide, just after the following paragraph:

After a first beta release is published, no new features are accepted. Only bug fixes can now be committed. This is when core developers should concentrate on the task of fixing regressions and other new issues filed by users who have downloaded the alpha and beta releases.

But, I think it wouldn’t hurt to also require the PRs to be explicitly labeled as type-bugfix or type-documentation in order to be merged into a beta branch (via status check). This at least indicates that some thought was put into defining it as a bugfix or documentation-only change. Perhaps in addition to that, miss-islington (or other repo bot) could provide a link to that devguide section as a final “Are you sure this is a change that should be merged into a beta branch?” reminder, when applicable. This could improve discoverability of the section significantly.

For the record, I added Py_DEPRECATED macro to APIs they have been deprecated in document and PEP and not used widely. (ref: 1, 2)
I am not 100% sure it is bugfix. If it caused trouble in downstream project and it is not easily fixed, I will revert it.

2 Likes

Keep in mind that most (but not all) bugfixes will be applicable to multiple branches. So, the normal flow is to apply the bugfix to the master branch which is always open to feature fixes and bugfixes. Then, if necessary, possibly after some exposure on the master branch and to the master branch buildbots, a backport to the pre-release branch (currently 3.9) and other bugfix (3.8) or security fix (3.7, 3.6, 3.5) branches can be proposed and, in many cases, executed with the addition of the appropriate backport labels to the original PR. So there shouldn’t be a lot of question about when something is appropriate to merge to master. Once the fix is securely in master, in general it’s a lot easier to handle the separate questions of if and when to backport.

2 Likes

I was aware of the main workflow of porting the change to master first and then considering backports, but I wasn’t aware of the practice of waiting for additional exposure on master prior to waiting to apply the backports. Typically, I’ve applied the backport label(s) more immediately for changes that seem applicable (usually after double checking with other core devs first, unless it’s documentation-only).

Should I be waiting a longer duration after merging the initial PR to master before starting the backports for code changes? In retrospect, it seems like a much better idea to at least ensure the buildbot jobs passed in post-commit (to master) before backporting. If there are any issues, I can see that it would be significantly easier to revert if the change is only present in master.

Sorry if I wasn’t clear in my previous post, but by “can be merged during the beta”, I really meant “can be backported to a branch in beta and merged” rather than “can be merged to master”. :slight_smile:

I think it’s a judgement call. I would factor in the perceived complexity of the fix, what areas of the code it touches, and where we are in the pre-release cycle. The point of this topic is to make sure we all are doing what we can to get the next feature release out on time and with high quality. The beta phase is a good time to be extra cautious with backports!

3 Likes

1/5 of the SC likes this idea. :wink: If you want to open an issue at https://github.com/python/steering-council to propose this idea then the SC can discuss it (probably with RM input).

1 Like