Core developer rules of thumb (incl. "What needs a PEP"?)

In the recent years, there’s been a new phenomenon in CPython development: core developers who work on Python full-time, including whole teams of them.

Overall, this is great: things can be improved and fixed much faster than before. On the other hand, since a few people can now move much faster than most of the others, we’re starting to get issues in coordination – ensuring that all relevant people can follow changes, review them, and suggest better alternatives. One of the strengths of a large open-source team is that we can represent many different use-cases and viewpoints, and we run the risk of losing some of that.

How can we improve the situation, without losing the advantages that dedicated teams and individuals bring? I want to suggest a few rules of thumb to keep in mind when contributing to the Python project. It applies to everyone, but if you have more time than the rest of us, consider spending more time than usual following them. These aren’t final; suggestions are welcome.

(This is based on discussions in the Steering Council, but as it grew in scope, I realized – perhaps too late – that it would work much better if I just propose it myself and gather community input, than gather SC agreement and then publish a document out of the blue.)

This is also available as a Google doc, anyone with a Google account may comment there.

Be prepared to revert or fix

Be prepared to revert your change if it has unexpected consequences. Think twice before making large-scale, complex changes: you should ensure you’ll have time to fix any follow-up issues.

It helps to communicate your changes ahead of time and ensure there are other people who understand the issue and changes.

Don’t run experiments in the main branch

If you have a major change that’s not ready to go in, do not make preparatory commits in main.

Such changes should either be part of your bigger change (outside the main repo), or have their own valid reason to be merged – and they should have tests, docs, or even a PEP, as appropriate.

We want to avoid two things:

  • Code for failed experiments being kept as additional baggage
  • Preparing a specific solution to a problem that might have a better solution

Examples of planned changes where this applies are:

  • GIL removal
  • subinterpreter support
  • moving C state to the heap

Respect others’ time

If you can dedicate time to Python every day, keep in mind that there are others who can’t, and who can nevertheless have good input. If a change isn’t blocking or time-sensitive, wait a week (or more) for feedback on pull requests or discussions. Especially if you ask for such feedback.

Keep a roadmap

If you have a long-term project, keep a roadmap document so others can see the “big picture” and suggest high-level ideas. Currently, a “whitepaper PEP” like PEP-659, PEP-620, PR-2212 is the way to do this. A PEP might not be the best format for such a roadmap (the SC is discussing potential alternatives), but it’s better to publish something that’ll move later than to keep the planned steps and goals as a surprise.

Note that Informational PEPs do not necessarily represent a Python community consensus or recommendation: you don’t need any sort of approval to publish one. But since they can’t be approved, they’re not enough to make PEP-sized changes to Python.

If it ain’t broken, don’t fix it.

It is too easy to introduce unwanted bugs when refactoring. Only do refactoring or code clean-up if you’re touching the relevant code for a different, better reason. (Or the refactoring is part of an accepted PEP.)

You almost certainly need a PEP for:

  • Changing the syntax
    • (except e.g. changing the error message for an existing SyntaxError)
  • Adding/removing/changing a new builtin
  • Adding or removing a stdlib module
  • A change that will introduce a visible, measurable performance regression

When you should open a project-wide heads-up or discussion:

A thread on Python-dev or Discourse is in order for:

  • changes that affect many different modules
  • breaking backwards compatibility (i.e. breaking users’ working code)
  • any change with which others will likely disagree

Be prepared to write a PEP: if you start an extended discussion, it should be summarized in a document.

An issue or PR is probably fine for:

  • Obvious bugs with straightforward or backwards-compatible fixes
  • Implementing an approved PEP, or a change with SC approval*
  • Adding API that you are ready to support for several decades
  • Documentation clarifications/typos
8 Likes

This is great, thanks for writing it up!

A few thoughts:

Be prepared to revert your change if it has unexpected consequences.

I think in general we should be quick to revert changes if they cause trouble (e.g., broken buildbots or similar). It’s important for main to always be in a good state.

Don’t run experiments in the main branch

Not sure about this. The alternative would seem to be a single monster PR that makes a big change at once. That’s risky because it’s hard to keep the branch up to date, and if something breaks it’s harder to figure out the precise cause.

Alternative idea: Experimental work can go into main if there’s general agreement about the direction. Emphasize that code for failed experiments should be cleaned up promptly.

2 Likes

The idea is that large changes should be broken up into steps, where each step is individually an improvement. IMO this approach works in a surprising number of cases – and if it doesn’t, the change should definitely be a PEP, so you can discuss with the whole team and explain why you need an exception to the rules of thumb.

Given the diverse nature of the core development team wouldn’t this include most changes?

Indeed.

That seems a little strong. I can imagine a large refactoring of the internals of some data type that would be hard to carry out and review in one large PR, so the reviewer/mentor proposes to do it in a number of steps. The first step might be a straightforward transformation that has little to recommend itself except that it makes it possible to do the next stage in several smaller steps, maintaining correctness in each case. But only the final version produces the aimed-for speedup. And yet the whole project is truly internal so shouldn’t need a PEP (and probably very few people would be interested in providing feedback – this is not a controversial change, just a large one).

Also if the rule is so strong that an exception requires a PEP, it’s hardly a “rule of thumb”. :slight_smile:

4 Likes

I would add a rule of thumb along the lines of: refactoring should not be done in the same PR with functional changes.

PRs that change functionality should have a diff which is as small as possible, to make them easy to review and later to debug if necessary. Non-functional changes that improve the structure of the code should be done separately.

This is explained very well in Martin Fowler’s ‘refactoring’ book (which I recommend).

I am aware that this may appear to be in conflict with the anti-refactoring rule of thumb, but I don’t think it is - I think it’s just that the anti-refactoring rule needs to be rephrased in a more nuanced manner.

7 Likes

In other words: Don’t refactor unless you are making other changes already, and then make the refactoring it’s own PR so the steps are easily discernible, reviewable, and repairable.

1 Like

By ‘other changes’ I assume you mean ‘functional changes’. I don’t know if that is the right rule. Refactoring can come before a functional change (to make it possible) or after a functional change (to reduce entropy, like remove repetition). Say you made a functional change that left some entropy, I noticed it and want to tidy up. I should be able to do it.

I think what we’re trying to define here is the line between useful refactoring (improving the structure of the code) and pointless code churn (aesthetic changes).

3 Likes

Are there any GitHub project management features, or say wiki we could enable for the purposes of large project roadmaping?

There’s GitHub Projects, which (as I understand) works a lot like a somewhat simpler but more integrated version of ZenHub epics.

Correct, although the beta features also up it a bit. We can also allow for projects at the org level instead of the repo level as well (which is what’s required for now if you want the beta features).

1 Like

Following an offline conversation about this, I think the document should define what is meant by ‘refactoring’, and in particular make a distinction between formatting, refactoring and re-implementing.

4 Likes

This might also extend to unexpected effects found in third-party projects.

And so we get changes merged in unilaterally, without consensus. Where’s the balance?

This section is meant for changes that are not ready to go in – experiments.
If the overall change isn’t controversial, but just needs to be broken up into several steps, then it should!
I’ll make that clearer, and combine with the suggestion to put refactorings in a separate PR.

There are, but tend to have issues with either

  • permissions (it’s in the python org, but people involved aren’t all core devs, and asking admins to invite people for temporary projects is awkward), or
  • discoverability (it’s not in the python org, and core devs don’t know about it).

So I’d go for a whitepaper PEP with a high-level summary plus an external “live” project board.

IMO, as a rule of thumb, even improving the structure is unwanted if it doesn’t accompany a functional change. It’s too easy to introduce bugs.
(Of course, you’re likely to find bugs or possible enhancements when restructuring. And rewriting code is a great way to analyze/debug it. But if it isn’t an improvement it doesn’t need to be checked in.)

Happy to make the distinction, but how should the rules be different ffor these categories?

Should there be explaining (adding/fixing comments)? That’s a non-functional change I’d love to get in.

I don’t believe this is the right rule of thumb to have. Every change (functional or not) risks breaking something. The question is how to mitigate the risk and when to take it.

A non-functional change can reduce the chance of later introducing a bug (because, for instance, it makes the code easier to understand and change by removing repetition. As a concrete example, I recently had to make the same change in 3 places because the ‘emit an instruction’ pattern is repeated in the compiler 3 times with minor variations. I want to make another PR to consolidate these 3 functions into one).

Functional changes, over time, inevitably make the code harder to maintain. If we don’t deal with this continuously via incremental refactoring then we end up re-implementing whole components when they reach a state where nobody wants to touch them anymore. That is more likely to introduce bugs.

8 Likes

Proposal: rename _PyInterpreterFrame struct as _Py_framedata is another example of this kind of readability/maintainability proposal - minimising the size of the functional diffs when splitting an existing data structure in two has resulted in the situation where it can be hard to tell in some new diffs whether the code is using the old structure or the new one, and that’s a potential problem as the two have significantly different memory management expectations.

Now, we may decide in that case that further naming churn isn’t worth the hassle, but I don’t think it’s self-evident that keeping the status quo would be the right outcome. I do think it’s reasonable to require a discussion on Discourse before making that kind of change, though. While the risk of generating an interminable bikeshed discussion is real, I don’t think that outweighs the potential benefit in others pointing out potential downsides to the change before it is made, as well as in having a more in-depth rationale for the change available than can be readily incorporated into a commit message.

2 Likes

I see both of these as part of the original change: you’re already touching that code, and want to clean up in a separate commit.

A refactoring change is a good change if it improves the structure of the code enough to justify its risk. Whether it cleans up something I just changed or it cleans up something that someone else changed 3 years ago doesn’t make much difference to whether it is a good change.

The only relevance I see to my other work in the same area is that the other work gives me credibility to do the refactor. If that’s what it is, then should the rule of thumb be ‘refactoring should only be done by core devs and experienced contributors who are deeply familiar with the code they are changing’?

1 Like

I think that’s close, but still marginally too strong: I think it’s OK for someone else to do the refactoring as long as someone familiar with the code agrees the refactoring is worthwhile and is willing to provide a review.

An old example of a major refactoring with agreements to review changes: getting Unicode support into the old Python 2 import system required a heap of refactoring to make the functional changes more feasible. My recollection is that Victor wasn’t all that familiar with the Python 2 import code before he started that project, but Brett and I had been working with it for longer, and both agreed the changes were worthwhile and then provided reviews at various points along the way.

An example of a change that was ruled out pretty much on this basis: the enhancements proposed to the mimetypes module in Issue 6626: show Python mimetypes module some love - Python tracker were never adopted because the functional enhancements were mixed in with a major structural rewrite and we didn’t have anyone sufficiently familiar with the code and the way people typically used it to make the call as to whether the refactoring would be safe to adopt or not.

3 Likes