See http://lists.llvm.org/pipermail/llvm-dev/2016-March/096529.html for
context.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Unit Tests
Event Timeline
Comment Actions
Looks like my original proposal so LGTM. Please wait for some more folks to chime in though.
| llvm/docs/CodeReview.rst | ||
|---|---|---|
| 55 | What is this ! that phab is showing in the UI? Looks like some non-ascii character? | |
| llvm/docs/CodeReview.rst | ||
|---|---|---|
| 55 | thanks! Strangely it does not print in git diff. | |
Comment Actions
This is a great policy, thanks @silvas and @mehdi_amini.
LGTM with one small change (inline).
| llvm/docs/CodeReview.rst | ||
|---|---|---|
| 57 | I'd say "should undergo". The way it's written sounds like something that "happens" not something that "should happen". | |
| llvm/docs/CodeReview.rst | ||
|---|---|---|
| 49–51 | The wording seems to imply that this would apply even for opinion driven disagreements. I would prefer a wording which implies something more concrete. Also, in a way, it seems like the post-commit review practice is in conflict with the wording here. As in, if concerns are raised post-commit, then the post-commit review practice requires the original author to address those concerns even post-commit. I would think a revert will only happen if the original author is not acting on/addressing the concerns. Even in such cases, the revert should be justified. An example justifiable reason can be that progress cannot be made with out a revert. There are of course other obvious reasons to revert, but I am assuming that this discussion is not about those reasons. | |
| llvm/docs/CodeReview.rst | ||
|---|---|---|
| 49–51 |
How to you handle this in pre-commit review? Feel free to suggest an alternative wording, but keep in mind the spirit (see below). In general the mindset is that there shouldn't be a difference whether I express my opposition to a patch 10min before you want to push it or 10 min after you pushed it.
This is a case-by-case: if someone has the kind of concerns that would block a patch from landing (like design concerns), revert is the right approach I believe.
There is an obvious justification: the patch should not have been landed. A practical concern is that revert become harder quickly as other part of the codebase start to build on top of the landed change. | |
| 57 | Will update! (Note that this is carried over from the existing doc, I didn't touch it other than reflow) | |
Comment Actions
Considering that you are doing this in response to D83088, I think we have pre-existing evidence that this isn't workable as-is. There has to be some obligation on the people "thinking there should be more review" to actually engage in review. In particular, if they have previously demonstrated that they cannot be relied on for this, then the rule should not apply.
I'm not sure how to phrase that and whether the phrasing is even necessary. In particular, I doubt that this distinction is necessary if we were to lower the bar as I've explained inline.
| llvm/docs/CodeReview.rst | ||
|---|---|---|
| 47–62 | This is too low a bar for reverting given how unresponsive reviewers are in practice. I would suggest instead:
This also makes more sense because it aligns with the second paragraph, which talks about "the community member who identified the problem". You can always think that more review is better, but unless there is something concrete, we err too much on the side of review limbo. | |
| llvm/docs/CodeReview.rst | ||
|---|---|---|
| 47–62 | Actually, I agree with this statement. "thinks that ... would benefit" is a really low bar. I originally read in the naive "standard LLVM approach" where developers are mostly sensible, but I agree with @nhaehnle in this one. Once we "encode" in a *policy*, there could be people taking it more literally. I would also try to put some protection on the author that they must be contacted and in case of delay, other reviewers must be contacted, etc. We really want to avoid people silently reverting commits because they don't like how something is implemented. | |
Comment Actions
You are refusing to adjust to the existing practices and revert: this is hardly an evidence of anything to me at the moment.
In particular, if they have previously demonstrated that they cannot be relied on for this, then the rule should not apply.
I don't know what this means or how there is anything practical about this, feel free to suggest more rules around this though through a new RFC.
I'm not trying to propose anything new here: these practices seems clear to most 4 years ago, and we have been working this way for a long time now.
| llvm/docs/CodeReview.rst | ||
|---|---|---|
| 47–62 |
Fair! What about (changes are in bold):
This addresses mainly that we shouldn't treat differently a review posted 10 min before pushing or 10 min after (or a post-commit review): revert fast is the default.
(the same can be said of pre-commit review about blocking a revision because "they don't like how something is implemented", seems like something to be addressed uniformly) This is in the parenthesis actually: (preferably by the original author, unless they are unresponsive). In practice people are reasonable about this I think, but we can tweak it further, what about changing the end to say:
We can also add an expectation of active engagement from the reviewer in paragraph that follows, by slightly changing it like this:
| |
Comment Actions
I don't want to "take sides" and I'm only looking at this review from what it is and my own experience in the community. But looking at D83088, I see a somewhat problematic review.
@dblaikie has a long list of deep reflections on the design and goals of the patch, which indicates to me that the patch wasn't clear to begin with. The patch was approved by @arsenm but the author didn't request a final check by David, who was the main reviewer, who himself later on (I assume as soon as he could), replied he still wasn't happy with the patch. If I was the author I would have reverted as soon as David ask for it.
In this particular case, the change is in the CFG, which is a core part of the compiler and touches so many things outside of its scope that it's almost impossible to "get it right" from the beginning without breaking bots, patterns, codegen and other people's work in progress. You just have to look at Chandler's decade old attempt at changing the pass manager to see that changes deep in the compiler take time, and are very bumpy.
If some CFG design is either wrong, or could have been better, the longer we take to revert the harder, or even impossible, it will be to do so. Too much code depends on it and by extending the window we may be creating an ecosystem of current patches now relying on that design to work. That is why we have the fast revert + reapply cycle. We don't have good pre-commit validation, so we must act swiftly on post-commit review and requests.
To me, personally, @silvas and @mehdi_amini's addenda to the policy make sense and clearly apply to this particular case.
| llvm/docs/CodeReview.rst | ||
|---|---|---|
| 47–62 |
Agreed. Our lax policy of review and approvals does occasionally lead to soft approvals and lack of diverse reviews, which often comes after commit when the patch lands on people's CI or development trees. We can't have it both ways. We either have a strong pre-commit validation, or we have a reasonable post-commit policy. Reverting a patch is *not* a reflection on the code quality or the author's competence, just the need for more reviews. The longer we take to revert the patch, the harder it is and the patch can always be re-applied later.
Good point.
Right! This is a burden on people that want to contribute, not on everybody else working on their own parts of the project. It's a similar burden we put on new projects, back-ends, front-ends, otherwise it does not scale.
SGTM. | |
| llvm/docs/CodeReview.rst | ||
|---|---|---|
| 49–51 | [As with my other comment, I am assuming we are not discussing the obvious cases. Also, I was not aware of the other thread before I started commenting here.] IIUC, the current policy of post-commit review assumes good faith actors. So, if a good faith post-commit reviewer has concerns with a commit, the good faith committer is expected to address them. How they are to be addressed is something the reviewer and committer should work out. It could require a revert, or it could just be a simple follow up fix. It should be left up to the committer and the reviewer to work out the way forward. This arrangement does not make the post-commit review carry any less weight than a pre-commit review. That said, if you want to prescribe how a post-commit review should be addressed/approached in a policy document, I have no problems with it. But, the wording should not allow bad actors to exploit it. That is why I say the prescription should be about more concrete situations. As in, prescribe exactly when a revert is required as apposed to a blanket rule to revert first. For example, does the post-commit reviewer have concrete suggestions for clear cut issues identified, or are they just listing their opinions. And, when the reviewer does have concrete suggestions, does it need a revert to address them, or can they be addressed in a follow up change? I am unable to see why a blanket revert first policy is the best approach. If you have some situations where a revert is the best way forward, then do share them here. | |
| llvm/docs/CodeReview.rst | ||
|---|---|---|
| 49–51 | Did you read the email thread I linked in the description by the way?
What kind of "bad actors" do you expect? Have you seen "bad actors" in the community so far?
My angle here is quite "simple": pre and post commit review should be aligned: if my concern with the design aspect of your patch would prevent you from landing it, then I should be able to also ask you to revert it if I think there is a design problem that can't be addressed in-tree. | |
| llvm/docs/CodeReview.rst | ||
|---|---|---|
| 49–51 |
Yes, I did :)
Ah, sorry! I had this tab open and forgot to refresh and missed your update which changed the wording to "they may ask for a revert". I wrote my reply based on the earlier diff which said, "the patch should be reverted promptly". Sorry again, and this latest diff LGTM. | |
| llvm/docs/CodeReview.rst | ||
|---|---|---|
| 49–51 |
For example, someone can ask for a revert without actually showing a path forward or raising specific concerns. For example, they can just say, "this landed without enough review, please revert." Should one revert in response? | |
| llvm/docs/CodeReview.rst | ||
|---|---|---|
| 49–51 | I would revert, and then ask them what would they like to review exactly here? Re-landing a patch is "cheap". It is also very contextual: we could imagine someone who has never contributed to the project (or to this subproject) asking to revert a patch without being able to formulate their concerns, but has this been a problem so far? How have we dealt with this in the past? We also added the explicit expectation for this person to engage in the review of the patch: so we're back to pre vs post commit review alignment. | |
| llvm/docs/CodeReview.rst | ||
|---|---|---|
| 49–51 | Indeed, very contextual. But if the original reviewer asked me to revert, providing reasons about why I should do so, I would most certainly revert immediately. Holding on to a patch in tree is harder to fix than re-apply later, and gets harder as time passes. | |
Comment Actions
I don't seem to be reading D83088 in the same light as you. To me, this proposal would have clearly solved that problem. David was the main reviewer and has given an extensive explanation on the issues he was still seeing with the patch that landed. This is as clear as it comes, that patch must be reverted as soon as possible.
There has to be some obligation on the people "thinking there should be more review" to actually engage in review. In particular, if they have previously demonstrated that they cannot be relied on for this, then the rule should not apply.
I do not understand what you see as a demonstration of lack of reliability is but I see none there. And I don't see it often in LLVM. Certainly not from David! And that review was no example of that either.
Plus, writing rules on vague concepts of reliability that are clearly interpreted by different people differently makes for bad policy.
Historically, all our policies err in the side of trust. We trust the reviewers have good intention and we trust authors will act upon it the best they can. I honestly would hate to see us moving into a position where we are second-guessing everyone's intentions. It doesn't scale.
I'm not sure how to phrase that and whether the phrasing is even necessary. In particular, I doubt that this distinction is necessary if we were to lower the bar as I've explained inline.
I agreed with the bar being low in your inline comment and Mehdi has fixed that. Please read again the text.
Comment Actions
Overall, I think the policy change proposed here isn't entirely unreasonable, but I do think it needs to be treated as a change of policy. Not everybody is necessarily aware of a 4 year old email thread; heck, I've been working on upstream LLVM and frontends for 5 years and I wasn't aware of this supposed policy where in reality, things weren't actually done according to what's written in the policy documents. If I don't know this after 5 years, how can anybody joining in the last 4 years possibly be aware other than through mere chance. What's written in the documents needs to matter, if only as part of making LLVM a welcoming and inclusive community. Hidden tribal knowledge is the opposite of those ideals.
Back to the proposal itself: The problem I'm seeing (with post-commit review in general, but especially with this change) is that it exacerbates the dysfunction and arbitrariness of the LLVM review process. That's a practical problem because erring too much on the side of stasis blocks progress on dependent work, especially if that dependent work requires collaboration between multiple developers.
I think this needs a solution. That solution can be a separate discussion, but I think it ought to be adopted before making the problem worse. I will try to find some time to write this up.
| llvm/docs/CodeReview.rst | ||
|---|---|---|
| 54–55 | This is incorrect. Reverting a patch can in practice block other development more than not reverting it. Please just remove this statement. | |
| 60–62 | This sentence is missing a verb. Maybe something along the lines of: "In cases where the problem is identified by a buildbot, a community member with access to hardware similar to that on the buildbot is excepted to engage in the review." | |
Comment Actions
That email isn't policy. It's the attempt to encode behaviour that was "common" at the time. The LLVM community doesn't create policy on emails, we only encode how we behave after a consensus is formed if that's a good idea to promote further or not.
where in reality, things weren't actually done according to what's written in the policy documents.
Don't conflate your experiences with "reality". Reality is what the thousands of us who work on LLVM have experienced, from different groups, companies, countries, cultures. LLVM is so large these days that it's entirely possible that people only interact with a small subset of the community that experiences it in a similar way as themselves.
I don't know why Sean's proposal didn't go through. It could have been because there was no consensus (which would promote your view) or just that someone drop the ball and it hasn't happened since.
The important thing, however, is that:
- LLVM behaviours are not *all* encoded in policy documents. Some of them are just things we do and no one has problems with it so we just let it be.
- We only encode into policy things that become contentious (like this review), and for that to be encoded, needs consensus (which we still don't seem to have reached).
- Every policy, every document, every behaviour will invariably encode the majority, not the exceptions. That's why we don't like strong policies, because that makes it hard for us to accept worthy exceptions.
What's written in the documents needs to matter, if only as part of making LLVM a welcoming and inclusive community. Hidden tribal knowledge is the opposite of those ideals.
I absolutely agree with this statement. I personally know Mehdi for a while and Sean for even longer and I'm sure neither of them want to propagate hidden knowledge. The very reason for Sean's original email and Mehdi's review proposal is to do exactly the opposite: to elucidate what our behaviour is, and reach consensus on its merits.
Back to the proposal itself: The problem I'm seeing (with post-commit review in general, but especially with this change) is that it exacerbates the dysfunction and arbitrariness of the LLVM review process. That's a practical problem because erring too much on the side of stasis blocks progress on dependent work, especially if that dependent work requires collaboration between multiple developers.
Post-commit review isn't going away, nor should be discussed here in this review. This is a fact of the size and scope of LLVM and the alternatives are far worse. Let's just accept it as a fact for now and discuss this review.
Having an arbitrary review process is *NOT* dysfunctional. It's just arbitrary. I would even say it's evolutionary. We do this because that's what the constraints have led us, and we have discussed alternatives and that's the best we could come up with.
Working upstream is *a lot* slower than downstream, and this is another fact that we cannot change, nor we should discuss here. It's a side-effect of having far too many people involved with widely different points of views, goals and responsibilities.
This means that you are responsible for pushing your changes with a lot more effort than if it was just a local project, but it also means that, once you do, it has a much larger, industry wide impact.
I think this needs a solution. That solution can be a separate discussion, but I think it ought to be adopted before making the problem worse. I will try to find some time to write this up.
You have exposed many issues, not all of them "problems". I'd advise caution before you start a long unwinding thread about many things. We can change small things much faster than big ones.
More importantly, I don't think the facts that upstream development are slower and messier than downstream or that LLVM's policies are arbitrary or that LLVM has strong post-commit review process will change soon, if at all, and no amount of discussions or policies will speed that up.
Not because it's the status-quo, not because the maintainers "said-so", not because "I know better". But because this is where we (all) got after discussing those topics non-stop for (almost?) two decades...
| llvm/docs/CodeReview.rst | ||
|---|---|---|
| 54–55 | What this means to convey is that there are far more people working on other stuff than any one of us are working on our stuff. Perhaps it needs to be rewritten, but the idea is valid. If a patch is still in discussion, even if it remains in the tree, than it would be disingenuous for people to commit other depending patches on top of that, so essentially, the discussion process will effectively lock your progress no matter where the patch is. But this is the process that all code reviews follow, upstream or downstream. It's the responsibility of the authors to show the value in their patches, not the other way round. | |
Comment Actions
my 2 cents:
meta-point: this discussion should be happening on llvm-dev. it's clearly outside the scope of a patch review at this point.
good faith: we have to assume everybody in the community is acting on good faith. As Renato said, it just doesn't scale otherwise. If needed, we can develop a "bad actors" policy if bad actors show up in practice. In my experience, everyone is acting on good faith, but sometimes some 1-on-1 communication is needed for two parties to see each other's perspectives and come to consensus; back-and-forth on a patch review isn't necessarily the most productive.
Comment Actions
I disagree strongly here.
Not everybody is necessarily aware of a 4 year old email thread; [...] Hidden tribal knowledge is the opposite of those ideals.
Hence why I'm making the policy explicit.
Back to the proposal itself: The problem I'm seeing (with post-commit review in general, but especially with this change) is that it exacerbates the dysfunction and arbitrariness of the LLVM review process. That's a practical problem because erring too much on the side of stasis blocks progress on dependent work, especially if that dependent work requires collaboration between multiple developers.
I don't necessarily disagree with this, but addressing the problem of code review practices and how to unblock such review is orthogonal 1) to the revert practice and 2) writing down the existing policy.
I think this needs a solution. That solution can be a separate discussion, but I think it ought to be adopted before making the problem worse.
This patch isn't making anything worse, it is instructing a wider range of people who haven't been very involved upstream about the current upstream expectations.
Comment Actions
I pinged the original thread on llvm-dev@, if anyone has concerns they can express them there of course.
This review is mostly about writing down the existing policy and so should focusing on the wordsmith of the text.
I agree that waiting months to get a patch in is an issue (a long lasting one!), and our lack of processes around making progress in situation where there is a lack of consensus: I'm interested in improving the situation on these topics as much as possible, but that seems orthogonal to the revert.
Comment Actions
Generally, I think this patch reflects the existing expectation in the community, when the post-commit feedback is provided promptly.
Right now the suggested change is worded too strongly: it says that the author is responsible to revert promptly in response to any revert request, no matter the age of the commit. Some old commits can be easily reverted (for example, one-line changes of defaults), some can't be (I'd love if if we could undo that commit in Clang that makes Expr a subclass of Stmt, but asking to revert that right now would be unproductive).
Comment Actions
@gribozavr2 thanks for catching this! The original patch had " a recently committed patch" but I lost it in the rewrite...
Comment Actions
@gribozavr2 the paragraph now starts with "If shortly after landing a commit, a developer expresses concerns ..." ; WDYT?
| llvm/docs/CodeReview.rst | ||
|---|---|---|
| 60–62 | Applied the suggestion as-is, thanks! | |
Comment Actions
"If shortly after landing a commit, a developer expresses concerns" makes it sound like the developer landing and expressing are the same person. Try "If a community member expresses a concern about a recent commit, and this concern would have been significant enough to warrant a conversation during pre-commit review, ..."
Comment Actions
I'm going to accept this as it codifies a lot of existing intent behind how the community works. This was also fairly universally accepted 4 years ago in the original discussion and nothing has changed.
There are always going to be cases that we can chat about more frequently, but this frames the intent nicely.
-eric
Comment Actions
Thanks Eric!
Renato let me know if you have more suggestions or if the current revision looks good to you?
@gribozavr2 I took your suggestion as-is so I suspect you're fine with this at the moment?
Added @dexonsmith because @reames and @hfinkel aren't readily available.
Comment Actions
Just gave this a final read-through and it looks great. Thanks for championing this Mehdi!
Comment Actions
To clarify my LGTM after reading more of the discussion above, I think the wording here captures the community practice, making explicit some rules that weren't necessarily well-documented before. (This does not seem like a policy change to me, just documentation of the existing long-standing policy.)
The wording seems to imply that this would apply even for opinion driven disagreements. I would prefer a wording which implies something more concrete. Also, in a way, it seems like the post-commit review practice is in conflict with the wording here. As in, if concerns are raised post-commit, then the post-commit review practice requires the original author to address those concerns even post-commit. I would think a revert will only happen if the original author is not acting on/addressing the concerns. Even in such cases, the revert should be justified. An example justifiable reason can be that progress cannot be made with out a revert.
There are of course other obvious reasons to revert, but I am assuming that this discussion is not about those reasons.