[Twisted-Python] major changes, release engineering, and learning cost
The nice thing about Twisted's compatibility policy is that developers, and even users, very rarely have problems when installing a new version of Twisted. While this is a nice benefit, the current strategy of developing features in a compatible way does have a couple of costs, and I'd like to see if we can address them without giving up the benefit. I have a suggestion for a process tweak which would hopefully mitigate some of the difficulties which arise due to the compatibility policy. When we add a new feature that supersedes an older one, or fix a bug in Twisted that involves changing behavior, the developer fixing it has to come up with a new name. If we have several behavior-changing bugfixes in the same subsystem, that means that developers using Twisted may have to learn about 3 different symbol names. Since we tend to avoid just suffixing names with numbers (for good reason, I think), they won't have to learn Bicycle, Bicycle2, Bicycle3, they'll have to learn Bicycle, then Footcycle, and finally Velocipede, and somehow infer that Velocipede is the newest/best name that they should be using, by reading the (hopefully clear, concise) warnings that come out of their unit tests. This came up again recently on a ticket about URLPath, <http://twistedmatrix.com/trac/ticket/2625#comment:16>, where a contributor suggested that it would be better to make a whole new module because it's easier for external developers to learn about that then learn about an individual method change. This of course raises the question: if we're going to have a whole new URL class, shouldn't it fix the (numerous) *other* bugs that we know about in URLPath? Up until now the objection to doing things this way is that it results in gigantic branches which are intractable to review. That's a good objection, but it leaves us with a false dichotomy; reliable software and painless upgrades with a random scattershot of new features that are hard to understand, or coherent iterations of technology which can't be effectively reviewed, and therefore can't be effectively quality controlled. I propose that we get the best of both worlds by changing the way we do reviews slightly. Right now every code review needs to be done on an entire feature going to trunk, and _all_ of the code going to trunk needs to be reviewed at once. I suggest that instead, we create "integration branches" for sensible constellations of features, and have a two-stage review process. For example, let's say I want to work on making URLPath good. There are several tickets addressing it: <http://twistedmatrix.com/trac/ticket/2093> <http://twistedmatrix.com/trac/ticket/2094> <http://twistedmatrix.com/trac/ticket/2625> For the sake of argument, let's all pretend these are all deeply interrelated and involve changes to behavior of existing methods. I think that is sort of true of most of these, but it would be far too verbose to talk about *how*, and get bogged down in that discussion. First, I'd make an integration ticket, let's call it #ABCD, describing how these features are related and a brief outline of the new API I propose which resolves them. Then I'd create an integration branch from trunk, for that ticket. From the #ABCD branch, I'd create a branch for #2093, and put it up for review. The reviewer would review #2093 as usual, citing any test coverage issues, documentation issues, etc. After the usual review process, when I get an "OK to merge", I would merge #2093 *to the #ABCD branch*, not trunk. I would repeat this process for #2094 and #2625, merging each to the #ABCD branch as they passed review. Finally, I'd put #ABCD itself up for review. At this point the process would differ a bit. Reviewers would be able to assume, at this point, that the potentially large body of code in #ABCD had already been reviewed, that the test cases were good, the documentation was reasonably clear, and the logic made sense. This final review would be a quick sanity check, to make sure the tests still pass and that there are no conflicts. I would like to strongly emphasize that this point in the process would be an inappropriate time for reviewers to start arguing with each other over what is required for the branch to land, disputing the original specification, etc; this is just an opportunity to spot potential regressions before it lands. Each ticket review for a component of the larger feature should be an opportunity to draw attention to the direction of the larger feature development and prompt discussion. This *might* be an appropriate point to note that some other behavior-changing feature had been left out, though. In the case that there *were* conflicts, this would be an opportunity to review the conflict resolution itself. (We saw a nascent version of this approach on some stuff related to <http://twistedmatrix.com/trac/ticket/886> and it was hugely painful because nobody was really sure what the process was supposed to be. So let's not do it like that again.) So: thoughts? Does this make sense as a policy change for facilitating the development of major new features or consolidating behavior-changing fixes into easier-to-understand units?
In an effort to learn Twisted, I am working on a set of small client and server applications which utilize PB as IPC. The server hosts a group of simple parameters that clients may monitor or set. For a client to monitor a parameter's value, it seems that the server should create the parameter as a cacheable. To allow a client to modify the parameter, a referenceable looks like the right path. Of course, I would like to consume cake as well as posses it, wanting properties of both cacheable and referenceable. Looking at the source code, it doesn't look like subclassing from both pb.referenceable and pb.cacheable is a good idea as each has different ideas on how to get jellied. Currently, I'm thinking about just creating the parameter as a normal object that contains two PB objects: a referenceable and a cacheable. The former handles incoming requests to change the parameter's value, and the latter keeping clients updated with the current value. Is that a sane approach, or am I reinventing the wheel? Thanks, Jason Valenzuela
On 23 May, 12:26 am, glyph@twistedmatrix.com wrote:
The nice thing about Twisted's compatibility policy is that developers, and even users, very rarely have problems when installing a new version of Twisted. While this is a nice benefit, the current strategy of developing features in a compatible way does have a couple of costs, and I'd like to see if we can address them without giving up the benefit. I have a suggestion for a process tweak which would hopefully mitigate some of the difficulties which arise due to the compatibility policy.
When we add a new feature that supersedes an older one, or fix a bug in Twisted that involves changing behavior, the developer fixing it has to come up with a new name. If we have several behavior-changing bugfixes in the same subsystem, that means that developers using Twisted may have to learn about 3 different symbol names. Since we tend to avoid just suffixing names with numbers (for good reason, I think), they won't have to learn Bicycle, Bicycle2, Bicycle3, they'll have to learn Bicycle, then Footcycle, and finally Velocipede, and somehow infer that Velocipede is the newest/best name that they should be using, by reading the (hopefully clear, concise) warnings that come out of their unit tests.
This came up again recently on a ticket about URLPath, <http://twistedmatrix.com/trac/ticket/2625#comment:16>, where a contributor suggested that it would be better to make a whole new module because it's easier for external developers to learn about that then learn about an individual method change. This of course raises the question: if we're going to have a whole new URL class, shouldn't it fix the (numerous) *other* bugs that we know about in URLPath?
Up until now the objection to doing things this way is that it results in gigantic branches which are intractable to review. That's a good objection, but it leaves us with a false dichotomy; reliable software and painless upgrades with a random scattershot of new features that are hard to understand, or coherent iterations of technology which can't be effectively reviewed, and therefore can't be effectively quality controlled.
I propose that we get the best of both worlds by changing the way we do reviews slightly. Right now every code review needs to be done on an entire feature going to trunk, and _all_ of the code going to trunk needs to be reviewed at once. I suggest that instead, we create "integration branches" for sensible constellations of features, and have a two-stage review process.
For example, let's say I want to work on making URLPath good. There are several tickets addressing it:
<http://twistedmatrix.com/trac/ticket/2093> <http://twistedmatrix.com/trac/ticket/2094> <http://twistedmatrix.com/trac/ticket/2625>
For the sake of argument, let's all pretend these are all deeply interrelated and involve changes to behavior of existing methods. I think that is sort of true of most of these, but it would be far too verbose to talk about *how*, and get bogged down in that discussion.
First, I'd make an integration ticket, let's call it #ABCD, describing how these features are related and a brief outline of the new API I propose which resolves them.
Then I'd create an integration branch from trunk, for that ticket. From the #ABCD branch, I'd create a branch for #2093, and put it up for review. The reviewer would review #2093 as usual, citing any test coverage issues, documentation issues, etc. After the usual review process, when I get an "OK to merge", I would merge #2093 *to the #ABCD branch*, not trunk.
I would repeat this process for #2094 and #2625, merging each to the #ABCD branch as they passed review.
Finally, I'd put #ABCD itself up for review. At this point the process would differ a bit. Reviewers would be able to assume, at this point, that the potentially large body of code in #ABCD had already been reviewed, that the test cases were good, the documentation was reasonably clear, and the logic made sense. This final review would be a quick sanity check, to make sure the tests still pass and that there are no conflicts.
I would like to strongly emphasize that this point in the process would be an inappropriate time for reviewers to start arguing with each other over what is required for the branch to land, disputing the original specification, etc; this is just an opportunity to spot potential regressions before it lands. Each ticket review for a component of the larger feature should be an opportunity to draw attention to the direction of the larger feature development and prompt discussion. This *might* be an appropriate point to note that some other behavior- changing feature had been left out, though.
In the case that there *were* conflicts, this would be an opportunity to review the conflict resolution itself.
(We saw a nascent version of this approach on some stuff related to <http://twistedmatrix.com/trac/ticket/886> and it was hugely painful because nobody was really sure what the process was supposed to be. So let's not do it like that again.)
So: thoughts? Does this make sense as a policy change for facilitating the development of major new features or consolidating behavior- changing fixes into easier-to-understand units?
So, to summarize, we could stage our code using more than just two branches (trunk + feature branch) in order to make larger changes easier to understand for reviewers while still making each change to trunk a coherent unit. This sounds fine to me. We need to work out some details (like, for example, I'm not sure trying to do this using subversion is such a good idea, and we want the process to be documented somewhere so we don't have a repeat of #886), but I think we should try it and see what happens. Of course, someone needs to work on something big before we'll have a chance to try it. I'm not yet convinced that `URLPath` is a good case for this, though. It's very little code, and a complete reimplementation (if even such a thing is needed) will likewise be very little code. Also, I don't think a complete reimplementation is needed here. Going back to the proposed workflow change, we should also be sure there's a clear condition under which the integration branch should be merged to trunk. And ideally we should still try to keep the lifespan of these things as short as possible. Jean-Paul
On Mon, May 24, 2010 at 3:35 AM, <exarkun@twistedmatrix.com> wrote: ...
So: thoughts? Does this make sense as a policy change for facilitating the development of major new features or consolidating behavior- changing fixes into easier-to-understand units?
So, to summarize, we could stage our code using more than just two branches (trunk + feature branch) in order to make larger changes easier to understand for reviewers while still making each change to trunk a coherent unit.
FWIW, we've been doing this on Launchpad for some years and it works out well. As a rule, we don't have the final "sanity check" review, since we have robot minions that check for conflicts and that the tests pass.
This sounds fine to me. We need to work out some details (like, for example, I'm not sure trying to do this using subversion is such a good idea, and we want the process to be documented somewhere so we don't have a repeat of #886), but I think we should try it and see what happens.
Using a DVCS would make it much easier. For example, Bazaar has plugins like loom and pipeline that are designed to manage a stack of changes. Also, +1 on the documentation. jml
On May 24, 2010, at 7:42 AM, Jonathan Lange wrote:
FWIW, we've been doing this on Launchpad for some years and it works out well.
Good to know.
As a rule, we don't have the final "sanity check" review, since we have robot minions that check for conflicts and that the tests pass.
This is useful information. Our buildbot isn't *quite* PQM, but then... there's a strong case to be made that Jean-Paul is himself a robot.
Using a DVCS would make it much easier. For example, Bazaar has plugins like loom and pipeline that are designed to manage a stack of changes.
Looms still scare me a bit, but my intent was definitely that a DVCS (and yes, most likely specifically bzr) would be involved. If you have any particular insight as to how one might go about doing that, it would be helpful. For practical purposes, until the bzr-ness of the buildbots is more exposed, I was thinking that developers could work in bzr but push changes into svn branches for testing. Does that sound workable?
Also, +1 on the documentation.
I think we should continue this discussion after somebody has done at least one branch like this.
I've said something in #twisted but I hadn't read this reply yet, so for sake of saving this for posterity, I agree with jml here. I think we're mostly being bitten because of a lack of software tools, in the form of svn and trac. Disclaimer: I really dislike svn since I never figured out how Combinator works. I really dislike trac. So, for the rest of this e-mail, let's pretend we're implementing a big new feature since that's the thing I tried to do to some extent. Major stuff could be a blueprint on Launchpad. Blueprints match a branch for the "big feature". So, we have the Twisted blueprint quantum-transmogrification and a branch lp:~lvh/twisted/quantum-transmogrification.
From that branch I create a bunch of branches of review units (if it turns out it's too big, I just branch again for a new review unit). So, I want to do something with entanglement: lp:~lvh/twisted/quantum-transmogrification-entanglement, and it's good, so someone reviews and sends it back.
lp's merge proposals let you do the code review in arbitrarily small chunks. So if the thing I do next is lp:~lvh/twisted/quantum-transmogrification-ftl-travel, and it turns out FTL travel is really really hard so I need two smaller branches lp:~lvh/twisted/quantum-transmogrification-tunnels and lp:~lvh/twisted/quantum-transmogrification-ansible. Both are good, so they get put back into ~lvh/twisted/quantum-transmogrification-ftl-travel. Each review would verify that all children (if any) have also been reviewed. So, the final review is pretty small, as suggested :-) This does not limit a developer's freedom to branch at will, because code review is opt-in (merge proposal), not opt-out. If you don't do it, that code in that branch isn't covered by a previous review, and must be reviewed later. How exactly code review coverage would work is somewhat of an open question and it's the obvious failure in this system. We use it in production and it turns out to not be a problem, because people always end up doing two things: 1) always branch at least once from the first branch off trunk (so branch off lp:~lvh/twisted/quantum-transmogrification). Net result: lp:~lvh/twisted/quantum-transmogrification only introduces code in the form of merges. 2) always do code review on branches being merged into your first branch off trunk (so everything merged into lp:~lvh/twisted/quantum-transmogrification has to be reviewed already) Note that our merges into trunk are automagic. If it's merged into a direct branch off of trunk and it satisfies some qualities (such as full test coverage :)), it gets put into trunk, and that gets pushed to production servers. No human interaction. Scary at first, but then you realize humans were already involved in the QC process extensively at every point -- doing it this way just makes them take testing more seriously :) I think a bug would be similar except the root would not be a blueprint but a bug. Laurens
On May 26, 2010, at 5:41 AM, Laurens Van Houtven wrote:
Major stuff could be a blueprint on Launchpad. Blueprints match a branch for the "big feature". So, we have the Twisted blueprint quantum-transmogrification and a branch lp:~lvh/twisted/quantum-transmogrification.
So, while I can definitely sympathize with a certain animosity towards trac, and I can appreciate the goals and sensibilities of launchpad, I will probably flat-out veto any required / process-driven usage of Launchpad blueprints. Bugs, features, enhancements, etc, are all units of work that need to be tracked, and it's better to have one kind of crummy interface for tracking _everything_ than three interfaces, even three good interfaces, for tracking little bits stuff in different ways according to arbitrary distinctions. (As someone recently opined to me, Blueprints are a giant complicated interface for pasting the URL to a Google Wave into a text field. We might as well skip the text field and just link straight to the conversation from a Trac ticket.)
From that branch I create a bunch of branches of review units (if it turns out it's too big, I just branch again for a new review unit). So, I want to do something with entanglement: lp:~lvh/twisted/quantum-transmogrification-entanglement, and it's good, so someone reviews and sends it back.
lp's merge proposals let you do the code review in arbitrarily small chunks. So if the thing I do next is lp:~lvh/twisted/quantum-transmogrification-ftl-travel,
lp:~lvh... isn't a verb. What do you do with that string? :)
and it turns out FTL travel is really really hard so I need two smaller branches lp:~lvh/twisted/quantum-transmogrification-tunnels and lp:~lvh/twisted/quantum-transmogrification-ansible. Both are good, so they get put back into ~lvh/twisted/quantum-transmogrification-ftl-travel.
Each review would verify that all children (if any) have also been reviewed. So, the final review is pretty small, as suggested :-)
The review wouldn't verify that the parent had been reviewed, though. If you started this process by writing a bunch of code in the q-t-f-t branch, *that* code would never have been reviewed; unless q-t-f-t needs to be reviewed in its entirety before landing on trunk. Which is precisely what I'm trying to avoid.
This does not limit a developer's freedom to branch at will, because code review is opt-in (merge proposal), not opt-out. If you don't do it, that code in that branch isn't covered by a previous review, and must be reviewed later.
This strikes me as placing a pretty nasty burden on the reviewer. The reviewer has to figure out if there are any commits that went only to the integration branch, isolate them, review them, get that branch into an OK-it's-reviewed state, while meanwhile other developers might be committing stuff to that branch and changing its contents, both from regular working commits and from reviewed merges. It sounds like a nightmare. Maybe bzr makes it easier than it sounds, but it sounds bad enough that even a big improvement would still be pretty bad ;).
How exactly code review coverage would work is somewhat of an open question and it's the obvious failure in this system. We use it in production and it turns out to not be a problem, because people always end up doing two things:
Who is "we"? What is "production"? Are you talking about Twisted or a hypothetical project which uses Twisted, or a fork of Twisted on Launchpad? Is this a hypothetical project or a real proejct? I am super confused.
1) always branch at least once from the first branch off trunk (so branch off lp:~lvh/twisted/quantum-transmogrification). Net result: lp:~lvh/twisted/quantum-transmogrification only introduces code in the form of merges.
That's pretty much what I'm proposing, except I don't actually care whether they're merges or patches or individual commits, as long as they've cycled through code-review properly.
2) always do code review on branches being merged into your first branch off trunk (so everything merged into lp:~lvh/twisted/quantum-transmogrification has to be reviewed already)
And this is what we already do.
Note that our merges into trunk are automagic.
(Again, who is "we", and by what mechanism are they automated? Are you proposing that we do this, or are you stating that some other people do?)
If it's merged into a direct branch off of trunk and it satisfies some qualities (such as full test coverage :)), it gets put into trunk, and that gets pushed to production servers. No human interaction. Scary at first, but then you realize humans were already involved in the QC process extensively at every point -- doing it this way just makes them take testing more seriously :)
Human interaction of some kind should definitely be required for Twisted. This is not just pushing some new widget to a web site; this is potentially pushing out new APIs that need to be documented and supported to a whole ton of developers. The whole point of the process modification I've proposed is to make sure that features arrive in releases as coherent, comprehensible whole pieces, not to allow things we can automatically verify (like docstring and test coverage) to be deferred to later merges. These properties of the code should still be verified on every merge to the integration branch; the interesting thing about the merge to trunk is the verification that the unit is a coherent whole (and in the case of a deprecation / replacement, that the replacement is a functionally adequate upgrade).
I think a bug would be similar except the root would not be a blueprint but a bug.
So, I'm really confused as to what the purpose of this message was - are you just describing how a similar workflow might work if we used launchpad, advocating that we switch to launchpad in order to implement this, advocating that we use launchpad for big features but *not* for other stuff, or ... what? If you're proposing a different functional modification to the existing process, can you do it without reference to tons of launchpad-specific terminology? Sorry if this comes off as a little flamey; I really am just confused as to what the point was. Thanks, -glyph
For clarity: I think Launchpad replacing Trac is a good thing. I realize that's a huge ordeal. However, I don't think the basic ideas are so different that it'd be impossible. As discussed on IRC, the main downside (aka why we can't do it right now) is lack of notifications, so it's hard to integrate stuff like buildbot yet, but that's being worked on. The idea I'm proposing is probably doable without Launchpad, but it's definitely much harder without bzr. Mixing bzr and svn, might work, but the developers definitely need to be using bzr because branching really can't be a pain for it to work. I have diagrammed the quantum-transmogrifier example that I tried to explain in the last email. http://bit.ly/aA20Qs On Thu, May 27, 2010 at 7:02 AM, Glyph Lefkowitz <glyph@twistedmatrix.com> wrote:
On May 26, 2010, at 5:41 AM, Laurens Van Houtven wrote:
So, while I can definitely sympathize with a certain animosity towards trac, and I can appreciate the goals and sensibilities of launchpad, I will probably flat-out veto any required / process-driven usage of Launchpad blueprints. Bugs, features, enhancements, etc, are all units of work that need to be tracked, and it's better to have one kind of crummy interface for tracking _everything_ than three interfaces, even three good interfaces, for tracking little bits stuff in different ways according to arbitrary distinctions. (As someone recently opined to me, Blueprints are a giant complicated interface for pasting the URL to a Google Wave into a text field. We might as well skip the text field and just link straight to the conversation from a Trac ticket.)
(was it dash? ;-)) I understand your point of view, but I don't think blueprints are that bad. I'm not saying blueprints aren't fat pointers to URLs, but I just don't think that would necessarily make them less useful. As far as arbitrary distinctions go: I'd think new features are blueprints, and bugs are bugs. It's not very arbitrary in my mind -- which is just a different way of saying "I can't think of any grey areas". (Yes, this means there are very few blueprints. I think that's a good thing :)) I think I understand the reasoning behind your opinion from a project lead/release management/developer perspective: both bugs and blueprints are jobs that still need to be done, similarly tracked for releases, and they both take developer time to be resolved. I don't think this reasoning is wrong. For both users and developers, I think thinking of bugs and new features as separate things makes sense. Furthermore, Launchpad has stuff like milestones and targeted releases, so I don't think the three-good-interfaces thing is really that prohibitive. Personally, I don't feel that split is bad for developers either. (FWIW: yes, I think Launchpad's Whiteboard feature needs extending and it probably needs a comment system. And once you do that, you might indeed wonder what the difference with bugs still is -- but I'm not arguing Launchpad is perfect, I'm arguing it's better than Trac ;-)) Even if blueprints are non-negotiable, I think most of what I said could just as well be applied to Launchpad bugs: you'd treat Launchpad bugs like you treat Trac tickets now. Merge proposals and the reviews they come with are properties of branches in Launchpad, and not of blueprints or bugs (IIRC). So, feel free to scrap blueprints, it's not that big a deal :)
lp:~lvh... isn't a verb. What do you do with that string? :)
Sorry, bad emacs VC mode habit. I meant 'create a branch lp:~lvh/...'
and it turns out FTL travel is really really hard so I need two smaller branches lp:~lvh/twisted/quantum-transmogrification-tunnels and lp:~lvh/twisted/quantum-transmogrification-ansible. Both are good, so they get put back into ~lvh/twisted/quantum-transmogrification-ftl-travel.
Each review would verify that all children (if any) have also been reviewed. So, the final review is pretty small, as suggested :-)
The review wouldn't verify that the parent had been reviewed, though. If you started this process by writing a bunch of code in the q-t-f-t branch, *that* code would never have been reviewed; unless q-t-f-t needs to be reviewed in its entirety before landing on trunk. Which is precisely what I'm trying to avoid.
Yeah, this is sort-of fixed in practice by my point (1) below, but requires some conscious effort and discipline from the developer. An alternative idea to just having merges of reviewed branches in q-t, would be to have the review of q-t be "all of the commits that aren't reviewed merges from other branches". That sounds really, really annoying, so I'd rather do it the first way. Specifically, that means "don't do that, branch early and often, merging is easy but branching halfway is confusing".
This does not limit a developer's freedom to branch at will, because code review is opt-in (merge proposal), not opt-out. If you don't do it, that code in that branch isn't covered by a previous review, and must be reviewed later.
This strikes me as placing a pretty nasty burden on the reviewer. The reviewer has to figure out if there are any commits that went only to the integration branch, isolate them, review them, get that branch into an OK-it's-reviewed state, while meanwhile other developers might be committing stuff to that branch and changing its contents, both from regular working commits and from reviewed merges. It sounds like a nightmare. Maybe bzr makes it easier than it sounds, but it sounds bad enough that even a big improvement would still be pretty bad ;).
Again, I think point (1) addresses this: yes, but not if you promise to make branches off your first branch off trunk (wording is a bit off, but look at the diagram for clarification). That way they only have merges from other branches, and those merges are reviewed. As long as you don't do that, and keep your development out of review branches, there is no problem. That sounds like a very big caveat, but we have found it to work in practice. I'm not sure why, but one explanation would be that people sometimes hugely underestimate how much time something takes to develop, but guesstimates about the complexity of a particular feature tend to be much more accurate. Even if that goes awry, there is quite some leeway here: the complexity of a review branch has to really completely get out of hand before it wouldn't be okay for it to be one code review anymore -- up to the point that it probably wouldn't pass review anymore under the old design. An added bonus is that there is reduced incentive to keep piling on features in a single review branch, because all of it has to be reviewable in one go. I think this is a good idea, because it encourages proper planning and up-front specs of which features you want to implement. This effect might be stronger in a small, tight development team such as in a small development house than with a distributed development team like Twisted (screwing over your reviewer just means he'll be less friendly to you next time you have to do reviews, and you still have to work with these people later on), but I'm going to be optimistic and pretend we're all nice people :-)
How exactly code review coverage would work is somewhat of an open question and it's the obvious failure in this system. We use it in production and it turns out to not be a problem, because people always end up doing two things:
Who is "we"? What is "production"? Are you talking about Twisted or a hypothetical project which uses Twisted, or a fork of Twisted on Launchpad? Is this a hypothetical project or a real proejct? I am super confused.
This is a real project that uses (amongst other things) Twisted.
1) always branch at least once from the first branch off trunk (so branch off lp:~lvh/twisted/quantum-transmogrification). Net result: lp:~lvh/twisted/quantum-transmogrification only introduces code in the form of merges.
That's pretty much what I'm proposing, except I don't actually care whether they're merges or patches or individual commits, as long as they've cycled through code-review properly.
Right, but wouldn't it then be hard for reviewers to know what has been reviewed and what hasn't?
2) always do code review on branches being merged into your first branch off trunk (so everything merged into lp:~lvh/twisted/quantum-transmogrification has to be reviewed already)
And this is what we already do.
Huh? I thought it got reviewed when it was put up for review for suggested merging into trunk. My suggestion is the same thing, except s/trunk/q-t/. There's a second review when q-t itself gets merged into trunk, but as long as those are all merges of reviewed branches, that review is trivial. See diagram, points 16 and 17.
Note that our merges into trunk are automagic.
(Again, who is "we", and by what mechanism are they automated? Are you proposing that we do this, or are you stating that some other people do?)
The aforementioned project using Twisted. I'd prefer not going into a lot of detail, maybe we should forget about it for purposes of keeping the discussing focused. Perhaps what _can_ be taken home from this for purposes of the discussion is that this way of organizing branches does actually work for at least one development team somewhere. I'm not sure to what extent this carries over to Twisted. All I know is that the distributedness of Twisted development isn't much of a problem, since I had no issues and I spent 99% working from home/university.
If it's merged into a direct branch off of trunk and it satisfies some qualities (such as full test coverage :)), it gets put into trunk, and that gets pushed to production servers. No human interaction. Scary at first, but then you realize humans were already involved in the QC process extensively at every point -- doing it this way just makes them take testing more seriously :)
Human interaction of some kind should definitely be required for Twisted. This is not just pushing some new widget to a web site; this is potentially pushing out new APIs that need to be documented and supported to a whole ton of developers. The whole point of the process modification I've proposed is to make sure that features arrive in releases as coherent, comprehensible whole pieces, not to allow things we can automatically verify (like docstring and test coverage) to be deferred to later merges. These properties of the code should still be verified on every merge to the integration branch; the interesting thing about the merge to trunk is the verification that the unit is a coherent whole (and in the case of a deprecation / replacement, that the replacement is a functionally adequate upgrade).
Yeah, I can see that. My point is not so much an argument for implementing automatic merging into trunk for Twisted, but mostly that this method, when properly implemented, results in a very high confidence level of the quality of your implementation branches, up to the point where people have successfully automated it :) As far as the coherent, comprehensible releases, that's one of the reasons I like Launchpad's milestones, series, blueprints... You want to do all of that and I think it's a great idea, and I think that they're good tools for making all of the specifics of that intent (coherent, comprehensible releases) more transparent to the outside world.
Thanks,
-glyph
Thanks, Laurens
On May 27, 2010, at 4:27 PM, Laurens Van Houtven wrote:
I have diagrammed the quantum-transmogrifier example that I tried to explain in the last email.
OK. With this diagram in mind, I can see that what you're proposing is nearly identical to what I've already proposed, except that you are being very vague as to the *requirements* on when and whether branches get merged. I'm not concerned with the "level of confidence" that you describe (we already have that), but with a modification to the *requirement* that branches be fully reviewed before a merge to trunk, and that the reviewer can block that merge. What I've suggested, simply put, is that we can have branches that land on trunk without being fully code-reviewed, *provided that each commit to that branch was itself code-reviewed*. There are a few fiddly details beyond that, but we seem to be in agreement on that broad picture. So, sorry for a terse response to a message that obviously took a long time to write, but I don't think this merits further discussion :). -glyph
When you say merge, do you mean into trunk, or also the submerges into my own feature/review branch? The big problem I can think of is that interfaces are something you should probably have a rough idea about way before any code gets written, but under this system branches get reviewed per feature set, so it takes a very long time before anyone (at least the reviewer, in the worst case also the developer) gets a half-decent view of how the entire thing is going to look when it's finished. I'm not saying interfaces should be set in stone, of course. I just think you should have some basic design that people agree on before you start writing tests, let alone implementation code. (I think this is where blueprints fit in). Laurens
On May 26, 2010, at 9:53 AM, exarkun@twistedmatrix.com wrote:
On 08:44 am, glyph@twistedmatrix.com wrote:
On May 24, 2010, at 7:42 AM, Jonathan Lange wrote:
Also, +1 on the documentation.
I think we should continue this discussion after somebody has done at least one branch like this.
Well, we did #886, right?
I don't think that counts. I meant that we've done at least one feature that *successfully* used a set of stacked branches. (I hope "stacked" means what I think it means here, and not some other crazy bzr thing.) #886 is more a description of the failure condition here than a success. #886 had one half-hearted sub-ticket, #3811, which eventually got abandoned and had to wait until #886 was merged to trunk anyway. And lots of other stuff that I wish had been added at the same time for a coherent unit of new functionality (like the high level API) was spun out into a separate ticket because it was too hard to review all at once. The high-level ticket in this case is really sneakily hiding out here: <http://twistedmatrix.com/trac/wiki/TwistedWebClient>. (A few things are still open there but I doubt that this is still a good candidate for development under the proposed style.)
On May 23, 2010, at 10:35 PM, exarkun@twistedmatrix.com wrote:
On 23 May, 12:26 am, glyph@twistedmatrix.com wrote:
So: thoughts? Does this make sense as a policy change for facilitating the development of major new features or consolidating behavior- changing fixes into easier-to-understand units?
So, to summarize, we could stage our code using more than just two branches (trunk + feature branch) in order to make larger changes easier to understand for reviewers while still making each change to trunk a coherent unit.
That's about the size of it.
This sounds fine to me. We need to work out some details (like, for example, I'm not sure trying to do this using subversion is such a good idea, and we want the process to be documented somewhere so we don't have a repeat of #886), but I think we should try it and see what happens.
Great. I propose some core committers try it out however makes sense to them, on whatever the next obvious thing to try it on is. Rather than try to document the whole process up front, please just spell out what you expect the reviewer to do on each ticket placed into review this way, and we'll document the process after we've nailed down something that works.
Of course, someone needs to work on something big before we'll have a chance to try it. I'm not yet convinced that `URLPath` is a good case for this, though. It's very little code, and a complete reimplementation (if even such a thing is needed) will likewise be very little code. Also, I don't think a complete reimplementation is needed here.
Yeah, like I said: I just grabbed that example because it was handy, not because I thought it was particularly appropriate. I don't even have anything in particular in mind. I actually wanted to bring this up while we were *between* major things, so that we could avoid discussions of specific problems with a current branch or feature (once something's in the middle of being implemented it develops a life of its own, and this is often an emotional context in which to talk about process changes).
Going back to the proposed workflow change, we should also be sure there's a clear condition under which the integration branch should be merged to trunk. And ideally we should still try to keep the lifespan of these things as short as possible.
My proposed criterion would be that the integration branch has an associated ticket, with links to a list of all other tickets expected to be a part of it. When all tickets on that list are closed, it can be merged at any time. This would, however, leave the door open for a reviewer to say "#XXXX is okay to merge, but based on my review really need to consider #YYYY before it can be merged to trunk, so please add that to the integration branch list". Of course, in the interest of keeping these lifespans short, this suggestion should be used sparingly. But it would be good for things like "update the documentation and examples" or "I noticed that the old system had feature X, we really need to keep parity with that before we deprecate it". I still like the idea of a final sanity check, but based on jml's feedback about Launchpad perhaps it would be best if we kept that step optional. Especially since I can't think of a clear set of guidelines for reviewers at that stage. (I mean, they *should* check for all the same stuff one normally checks for; coverage, documentation, etc, but they *shouldn't* block the merge from going to trunk while they re-audit every changed line of code, as that defeats the purpose of having incremental reviews in the first place.)
It would already be a big step forward if the naming convention could be addressed. Unfortunately the web vs. web2 thing really puts code archeologists in a confusion if the incremental numbered module policy is applied, because it appears that web2 > web but it is actually not. code archeology could be less of an issue if some of the following would be found good: 1) for existing developers, release notes would keep them up to date. Also, perhaps a migration guide on how to move from old to new would ensure people are using latest and greatest. 2) write a module guide for new developers, saying don't use X because it is old. Use Y 3) once new module is there, raise decpreciation for old module. As the old could be still valid, make a warning suppression mechanism. 2010/5/23, Glyph Lefkowitz <glyph@twistedmatrix.com>:
The nice thing about Twisted's compatibility policy is that developers, and even users, very rarely have problems when installing a new version of Twisted. While this is a nice benefit, the current strategy of developing features in a compatible way does have a couple of costs, and I'd like to see if we can address them without giving up the benefit. I have a suggestion for a process tweak which would hopefully mitigate some of the difficulties which arise due to the compatibility policy.
When we add a new feature that supersedes an older one, or fix a bug in Twisted that involves changing behavior, the developer fixing it has to come up with a new name. If we have several behavior-changing bugfixes in the same subsystem, that means that developers using Twisted may have to learn about 3 different symbol names. Since we tend to avoid just suffixing names with numbers (for good reason, I think), they won't have to learn Bicycle, Bicycle2, Bicycle3, they'll have to learn Bicycle, then Footcycle, and finally Velocipede, and somehow infer that Velocipede is the newest/best name that they should be using, by reading the (hopefully clear, concise) warnings that come out of their unit tests.
This came up again recently on a ticket about URLPath, <http://twistedmatrix.com/trac/ticket/2625#comment:16>, where a contributor suggested that it would be better to make a whole new module because it's easier for external developers to learn about that then learn about an individual method change. This of course raises the question: if we're going to have a whole new URL class, shouldn't it fix the (numerous) *other* bugs that we know about in URLPath?
Up until now the objection to doing things this way is that it results in gigantic branches which are intractable to review. That's a good objection, but it leaves us with a false dichotomy; reliable software and painless upgrades with a random scattershot of new features that are hard to understand, or coherent iterations of technology which can't be effectively reviewed, and therefore can't be effectively quality controlled.
I propose that we get the best of both worlds by changing the way we do reviews slightly. Right now every code review needs to be done on an entire feature going to trunk, and _all_ of the code going to trunk needs to be reviewed at once. I suggest that instead, we create "integration branches" for sensible constellations of features, and have a two-stage review process.
For example, let's say I want to work on making URLPath good. There are several tickets addressing it:
<http://twistedmatrix.com/trac/ticket/2093> <http://twistedmatrix.com/trac/ticket/2094> <http://twistedmatrix.com/trac/ticket/2625>
For the sake of argument, let's all pretend these are all deeply interrelated and involve changes to behavior of existing methods. I think that is sort of true of most of these, but it would be far too verbose to talk about *how*, and get bogged down in that discussion.
First, I'd make an integration ticket, let's call it #ABCD, describing how these features are related and a brief outline of the new API I propose which resolves them.
Then I'd create an integration branch from trunk, for that ticket. From the #ABCD branch, I'd create a branch for #2093, and put it up for review. The reviewer would review #2093 as usual, citing any test coverage issues, documentation issues, etc. After the usual review process, when I get an "OK to merge", I would merge #2093 *to the #ABCD branch*, not trunk.
I would repeat this process for #2094 and #2625, merging each to the #ABCD branch as they passed review.
Finally, I'd put #ABCD itself up for review. At this point the process would differ a bit. Reviewers would be able to assume, at this point, that the potentially large body of code in #ABCD had already been reviewed, that the test cases were good, the documentation was reasonably clear, and the logic made sense. This final review would be a quick sanity check, to make sure the tests still pass and that there are no conflicts.
I would like to strongly emphasize that this point in the process would be an inappropriate time for reviewers to start arguing with each other over what is required for the branch to land, disputing the original specification, etc; this is just an opportunity to spot potential regressions before it lands. Each ticket review for a component of the larger feature should be an opportunity to draw attention to the direction of the larger feature development and prompt discussion. This *might* be an appropriate point to note that some other behavior-changing feature had been left out, though.
In the case that there *were* conflicts, this would be an opportunity to review the conflict resolution itself.
(We saw a nascent version of this approach on some stuff related to <http://twistedmatrix.com/trac/ticket/886> and it was hugely painful because nobody was really sure what the process was supposed to be. So let's not do it like that again.)
So: thoughts? Does this make sense as a policy change for facilitating the development of major new features or consolidating behavior-changing fixes into easier-to-understand units?
_______________________________________________ Twisted-Python mailing list Twisted-Python@twistedmatrix.com http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
-- Nosūtīts no manas mobilās ierīces -- Konrads Smelkovs Applied IT sorcery.
participants (6)
-
exarkun＠twistedmatrix.com -
Glyph Lefkowitz -
Jason Valenzuela -
Jonathan Lange -
Konrads Smelkovs -
Laurens Van Houtven