Reminder of our Code Review Guidelines

#1

It’s a bit saddening to have to post this reminder, can everyone ensure they’ve read the code review guidelines. One of the most important things to consider here is the following extract from the original post (Improve Code Review Guideline):

Looking at the list of merged PRs against main (ignoring unity as that’s been clarified as intentionally disregarding the guidelines), it’s clear that this isn’t being respected as PRs are being merged without giving any visibility to the wider community:

Allow some breathing space when introducing changes, this is an open community and we should be inclusive - even if it means just holding a PR open for 24 hours so every time zone has seen it or waiting for the end of the next working day rather than merging when people aren’t available. Remember, we want diversity of thought, allowing the views of many to help influence the software we build rather than excluding those who may have great insights from ever noticing a change was made.

6 Likes
#2

Thanks for bringing this up. As a community we would like to constantly looking at the PRs at a case by case basis. How to bring agility with quality would be a key topics that we should all consider. This is even more important as we enter a competitive era of MLC.

As a community that also comes with volunteers, being able to work at different time point(including weekend) is also important, since many of our volunteer contributors in the community brings changes during only during weekends. The overall guideline does not come with a “no weekend policy” as a result. Instead, we trust our members to keep the guideline in mind, and use their judgement while open for followup feedbacks.

In the meantime, it is indeed important to ensure that relevant people in the respective areas are being informed, or be able to provide quick feedbacks afterwards. That does not entail that everyone should look at every PR, as a big project we love to empower many people in different areas to gain parallelism.

In those cases, we can also look from a pragmatic result driven POV. If a merge of the code results in the code in a better place and likely less controversial, we would expedite such merge while being accountable for possible problems arises here. There are great deal of opportunities to continue comment on the thread, bring up followup fixes, and iterate.

So specifically for the list of PRs, we would instead ask the following constructive questions and enable flexibility for our community and contributors. For example, I would ask does the PRs get review from the people who are familiar with related area of expertise? Are there any possible outstanding changes that needs to be made, or do they help the overall codebase in general? Is the merger open to followup when additional comments are raised.

Personally I think it is a great thing that many of the PRs, which relates to fixes and upgrades are merged in a timely manner. And I would like to thanks to our contributors to bring agility with quality to the project, and thanks to the committers who leverages the autonomy to help bring the parallel momentum.

It is also good to enable other means of interactions, for example, followup comments and forum interactions are great way to help (rather simply stop at PR merging pt, which can increasingly becomes problematic). Trying to followup too rigid view also resulted in another bad state, where it is harder to have followup conversations after a PR is merged. Everyone can still participate, and should participate after PR merging. We all share the same goal of enabling continued broad participation, continued interaction with the overall community.

Now coming back to the specific list of PRs. I went through them to double check. Seems most are fixes that are non-controversial and likely they also won’t cause issues to the rest of the developer community. In contrary, they help improve life of many. They are also reviewed by people who should be open to followup conversations if problems arise.

So for this specific set I think many of them are positive. If there is a particular example about what can be improved in some of the PRs, or a particular merge caused project codebase into an un-stable state, please do bring them up, and we can visit these cases accordingly and get followups.

For major architecture changes like those in unity, we would indeed anticipate a longer time period of interaction, example discussions and engagement to get broader buy-in as be stated in the guideline. And when to haste merging of these kind did happen, we can then come back and work as a community, to either do revert or post hoc revisit.

6 Likes
#3

Strongly agree with this. I would love to advocate once more that as a community, we promote open-mindedness and collective understanding so that no one is a bottleneck.

I took a brief look at the PRs you called out, many of which are from voluntary contributors:

I would love to provide my view as follows:

  • All of the PRs are contributed by voluntary contributors to enhance our codebase and fix existing issues;
  • All of the PRs got reviewed and approved by people who are familiar with the relevant area, who are willing to follow up if additional comments are raised;
  • All of the PRs follow our existing design and do not bring in any contradiction from the mergers’ point of view.

I would love to further remind us about the trust building in our code guideline:

As a community, the codebase is not owned by one company or two, and the code change does not have to be approved by specific companies. Instead, we trust and welcome voluntary contributions, and given that the mergers are experts of relevant fields, I would personally prefer trusting them to callig them out.

I know building a community is hard and different from running a team in a company, and let’s work together to make this happen :slight_smile:

2 Likes
#4

While I agree that some listed PRs should open for a longer time, most PRs listed here don’t add new features but just fixing bugs, adding/refactoring tests, etc. Do we really need, and does that make sense to simply enforce every PR being “visible” by the entire community? I don’t think so, and that’s why this is a “guideline” instead of a “policy” or a “rule”.

2 Likes
#5

As one of the authors of the PRs, I am quite surprised to be called out publicly, astonished and saddened to be conveniently characterized as anti-policy for my volunteer work in the open source community.

In my case, PR #14020 was reviewed by two committers who understand the technical details in the change. The original implementation was not perfect, so it was not accepted until multiple revisions according to the two committers’ feedbacks. During the process, my experience overall is moderately smooth (besides CI being too slow) and I believe the review process is nice because it caught all the potential issues in my PR.

holding a PR open for 24 hours so every time zone has seen it or waiting for the end of the next working day rather than merging when people aren’t available… help influence the software we build…

Please allow me to respectfully bring up the perception of distrust in your proposal: Every PR has to be discussed by all committers to prevent disastrous and irreparable damage to the software, even if it’s a single line typo fix and approved already by other knowledgeable committers.

As a contributor, I was shocked to read about it as it explicitly suggests two committers who are familiar with the PR are not enough to merge a PR. Also, as a committer, I was personally quite discouraged to be excluded from “the ones who build this software” because it suggests that my contribution has to be reviewed by “the ones who really build this software”, and my contribution cannot proceed when “people who are really building the software“ are not available because other committers’ approval does not count.

I would love to urge all of us to sympathize and acknowledge the contribution from all of the community members. Concretely, I found this mode of action has been working pretty well in the past: if committers believe a PR doesn’t bring in controversial changes, then just get it in; if there is anything wrong with a PR, then the merger is responsible for simply fixing it. It has been helping us include all those who cannot work on TVM full-time.

Anyways, Iet’s stop finger-pointing and please really build trust with the community. If we don’t trust our contributors/committers’ best judgement, it would be challenging for us to grow as a community effort.

4 Likes
#6

Hi folks !

Not sure on why PR #14036 got attention here, but after reading the topic permit me some few thoughts:

I am a contributor of TVM who always find pleasure to contribute to this wonderful community. Due to limited time, my contributions are sparse, usually driven by urges to improve obvious things around.

There is a special “flair” in the openness of TVM community, a unique one among other projects, thus it is always a pleasure to read other’s reviews, comments or push-requests even on any random topics.

This does not happen with other community driven projects (usually those backed by big-tech).

I also appreciate the time and dedication on the review process of the peoples here (regardless their role), it is always productive, and uttermost all are acts of real humans not just some “mechanical processes”.

As for #14036 I just picked up and older (stalled) PR #12144 owned by Jiabei Zhao, with a brief mention of very this, extended it to cover autotvm parts, updated releng requirements (as requested), setup a proper title (to get in the teams for review), manually Cc the original author (first) and some of original PR people. The tvm-bot Cc-ed other reviewers.

The PR got merged, however I expected more challenges about potential CI/RelEng implications of such changes, but got green on relevant required tests.

Underlined to the original author (Jiabei) the credits on his original work in the final comment.

2 Likes
#7

Thanks @junrushao, this highlights the need for maintaining visibility for all contributors. Rapidly merging changes without visibility erodes trust, despite our best intentions to get things done. I’d caution you from discouraging people calling out this behaviour, people should be encouraged to work towards what we’ve set out as guidelines within the community as they impact more than just ourselves.

As someone who volunteers my own time, I appreciate when my patches land, but they don’t have to land immediately - trust can be built over time :smile_cat:

I don’t think I ever excluded anyone from “the ones who build this software”, as I never used that phrase and I’d hope I never would. If you can point me to where I did I’d be happy to correct it.

I think you’ve highlighted a failure on my part @cbalint13 / @zxybazh , I should’ve mentioned that the list is just the last merges into main with a bit of scripted processing to get times - there’s no filter, it’s just data demonstrating the pattern.

Though we could get into the specifics of each PR, I’m keen not to dissect every patch in the list and argue whether we agree or disagree. As that’s not the spirit this post intended.

Thanks @comaniac, this is the spirit it was intended, I agree that this is guideline and we should approach with an open mind (14051 strikes me as a humorous inclusion in this list). This was an effort to highlight the community drifting from the established guidelines (as per vote) and was posted with best of intentions to encourage people to go back and re-visit them. It is concerning members of the community are actively discouraging following the guidelines in favour of their own approaches, further dividing the community.

3 Likes
#9

Thanks, @cbalint13, for bringing up an excellent point.

We are all humans, and it is the warmth of the community that attracts us under a common vision, continued collaboration, conversation, engagement, and less about mechanical-interpretation of things.

The guideline is intended to be used in individual PRs with constructive technical conversations under the context. Indeed, guidelines themselves can result in different approaches in each context to achieving the goal of agility, participation, and quality. The guideline itself is not intended to be mechanically interpreted, as such interpretations usually are not accurate reflections of the guideline.

One thing to keep in mind is that there are also multiple ways to achieve the same goal. For example, asking mergers to follow up on possible mistakes(if they do concrete arise) and encouraging continued conversation is a great way to enable visibility and continued participation of all community members.

We should avoid reasoning like “if we do not do X, then it is not Y” or “X is the only way to achieve Y”. There are a lot of different ways to collaborate in the same community under different contexts that still align with the overall guideline. Forcing everyone in the community to mechanically follow X can lead to misinterpreted results, stagnation, or exclusion of other approaches to get to Y. We may become more homogeneous through such a mechanical process, and that way works under, say, a single corporation setting. That usually comes with the cost of a less diverse and less active community, with a lost of momentum, fulfillment in each individual participations and continued conversations. Looking at each case under each specific context and engaging in constructive continued conversation is the way for the OSS community to thrive and grow. If there is a specific thought of possible misalignment, bringing conversations with each specific context (for example, “this PR contains this specific technical issue, and here are possible ways to improve it”) helps a lot.

Let us continue to have constructive conversations under each specific context. Welcome PRs, bringing code reviews, has continued engagement(and overall visibility) with the community before/during/after the code reviewing.

3 Likes
#10

@tqchen, these do not achieve the same goal, it is unwelcoming to enter code review after the code is already merged before getting a chance to see it. We can potentially expect some people to post after the fact, but we shouldn’t set the expectation for a volunteer to come across a closed PR and revive it with comments. This may achieve the same mechanical outcome of getting code reviewed and merged, at the cost of being unwelcoming to those who could’ve been engaged.

I don’t think there’s any intent on forcing everyone to mechanically follow anything, the code review guidelines are something which the community voted on, demonstrating consensus - I also admitted 14051 strikes me as a humorous inclusion in this list :smile_cat: clearly there’s flexibility. This was an effort to highlight the community drifting from the established guidelines (as per vote) and was posted with best of intentions to encourage people to go back and re-visit them as we’ve had many patches which didn’t need to be merged so fast.

The responses in this thread are fairly demonstrative of unwelcoming behaviour to those who wish to engage during code reviews, I do not think we should continue this unwelcoming behaviour. I didn’t expect such a vibrant response as this, I refer back to the comment of @comaniac:

This is all it takes to continue to welcome code reviews in our community, I don’t understand why we’re all not showing this level of courtesy.

#11

Thanks, please do continue to have constructive conversations under each specific context. Such engagement will not discounted at the state of the code themselves, be at code review or after.

For example, giving comments like: “this PR contains this specific technical issue, and here are possible ways to improve it” concretely helps to improve such a modules would be a great starting pt to have such a conversation that helps each PR, and of course for relevant PRs the folks will be mindful to get people engaged and involved during, or after, depending on the nature of the change.

1 Like
#12

Same here. Let’s strive to do our best concretely for the community by giving concrete technical feedbacks and trust each other’s best judgement :slight_smile:

There are ~150 PRs open, and many of them are there dead not receiving any follow-ups. I have to admit I don’t have time to help with all of them, and honestly would be super happy if one could review and merge those PRs ahead of me :slight_smile:

#13

@tqchen / @junrushao I tried to follow your proposed workflow and raised a specific issue within https://github.com/apache/tvm/pull/13831, there has been no reply and the author has been active elsewhere - what further action can be taken here?

#14

@Mousius caught a flu last week and was not active anywhere. To be clear, for small patches like removing unused code like you commented, please feel free to submit a PR as we always do :slight_smile:

#15

@junrushao, this is technical debt introduced by poor code review practices for which I’ve asked for a follow up; this thread indicates it’s the responsibility of the merger to follow up, are you now suggesting that’s not the case?

#16

@Mousius I have to disagree with your convenient characterization of my code being technical debt. The code is well organized and the architecture is well designed for future extensibility and is approved as RFCs. However, I don’t think this is anywhere I should spend my time arguing with you on :slight_smile:

this thread indicates it’s the responsibility of the merger to follow up, are you now suggesting that’s not the case?

I am sick in bed at the moment, but it’s only a few lines of unused code. if you think it’s urgent, sure, I can do it now

#17

Thanks Folks. Thanks junru for chiming in and please stay well rested and recover soon, I will cover this and do the PR for you

#18

Introducing unnecessary complexity and unused code paths (the Relax RFC has not been accepted and work is happening on the unity branch not main) is objectively technical debt, I don’t think there’s much to argue.

I’m sorry about your current health, and I do hope it improves. I never suggested that you should do the follow up right now, initially I was querying due to activity elsewhere and I further responded to your comment that I should follow up for you; I was concerned we were pushing responsibility of these follow ups on others. I’d be more than happy for you to follow up when you’re feeling better.

#19

I am not concerned about taking these follow up responsibilities, as @junrushao also covered a lot for myself as well, as a community we help each other. Thank you everyone for chiming in

#20

While I won’t take it personally and continue taking positive steps towards building a better software, I would love us to stop finger-pointing and start doing real work together - the flexibility we have and the appreciation we hold is how the community works :slight_smile: :heart:

2 Likes