What does Optimistic Merging mean for the Rust track? C4 and OM

[ghost]

hello @exercism/rust!
To improve contributor experience @ErikSchierboom introduced the idea of Optimistic Merging (OM) in #1482. It comes out of the C4 from Hintjens and the ZeroMQ project. C4 stands for "Collective Code Construction Contract". I find C4 and PM compelling and would like to define it in practical terms. I hope we can use this discussion to reach consensus should there be any disagreement.

I want to do this to improve contributor experience. By articulating our merging strategy we set expectations. That will benefit any contributor. It will also unify our team communication (e.g. one maintainer reviewing). I also think it is necessary for so we can iterate more quickly to V3. As mentioned in weekly calls, the Rust track is lagging behind others.

I believe we have been implementing a Pessimistic Merging strategy thus far. Hintjens details a number of problems with that approach. I cherry-pick a few I've observed or experienced:

It tells new contributors, "guilty until proven innocent," which is a negative message that creates negative emotions. Contributors who feel unwelcome will always look for alternatives.

It slows down the learning cycle Innovation demands rapid experiment-failure-success cycles. Someone identifies a problem or inefficiency in a product. Someone proposes a fix. The fix is tested and works or fails. We have learned something new. The faster this cycle happens, the faster and more accurately the project can move.

It puts the burden of work on individual contributors, which is ironic and sad for open source. We want to work together yet we're told to fix our work alone.

In my below wording I will use contributor for anyone involved with getting us to v3. (Track maintainer, misc contributor, etc).
Given the above critiques I believe we need to make the following changes:

• Approve and merge incomplete work instead of continuing lengthy discussion and critique about whether the PR sufficiently implements Rust idioms
  • My [Rust] start tuples concept exercise #1586 PR is an example of this. I felt defeated. I spent 2+ hours of my precious free-time on the task and the bulk of the work was rejected or rewritten. Were I not a maintainer I probably would've stopped contributing to v3.
• Contributors noting improvements should log them as separate issues and label them appropriately
  • I propose we do not use the suggestion feature of Pull Requests but instead, the contributor with suggestions should create a new issue. Then, the contributor submits a PR to address this feedback and can optionally request a review from the original contributor. This makes for the "happy patch party"
• We adopt point 14 of C4:

Maintainers SHALL NOT make value judgments on correct patches.

This would mean if it compiles we keep it. The more idiomatic or terse way of solving the exercise should follow the typical improvement issue + PR process as described above.

Thank you for reading this far. In summary I propose we optimize for any contribution and decrease our "change latency". This is in contrast with our current values of ideal Rust and abstract conceptual coherence (apriori DAG versus completed Concept Exercises. See #695).

What I need from you is to 👍 or 👎 as to how you feel and think about this change. Beyond that, do type out anything else. Thank you!

[petertseng]

You may discount my opinion, as despite nominally holding a maintainer label, I have not contributed meaningfully to v3. And thus, practically, should not be considered to have been a maintainer for the past few months. But even as a non-maintainer, I feel I can add an opinion that is not completely useless.

Someone once shared https://google.github.io/eng-practices/review/reviewer/standard.html with me. Although there is a bit to read here, the one point that stuck most with me is the bolded point in the "The Standard of Code Review" section, which I'll now quote.

In general, reviewers should favor approving a CL once it is in a state where it definitely improves the overall code health of the system being worked on, even if the CL isn’t perfect.

My quoting this doesn't mean that I think reviewers have asked for perfection in the past (in fact, since I have not followed most PRs here, I'm not even qualified to say whether that statement is true or false); it simply means that reading this quote has affected how I've approached my reviews ever since having read it.

Of course, we should understand that what constitutes an improvement may vary depending on the exact project. Some projects might, by their nature, have different requirements for being an improvement than other projects. I believe that is what is about to be discussed.

[ghost]

Thank you Peter. I think any track maintainer has a say here. A loose interpretation of OM would include that document. While justified differently I see their articulated principle very close to the spirit of OM.

[coriolinus]

Short answer: I disagree with the premise. C4 is interesting, but the specific process changes you suggest here seem antithetical to correct software development. At a high level, I do not believe that there is value in sacrificing correctness in favor of iteration speed.

Let's get to details.

[The Rust track has] been implementing a Pessimistic Merging strategy thus far.

Agree. I find this analogous to what the Rust compiler does: it only accepts programs which are correct according to its metrics, and the borrow checker in particular is more strict than most other widely used languages. Strictness is its strength. At the same time it provides useful error messages.

It tells new contributors, "guilty until proven innocent,"

It tells new contributors that there is a standard of quality in the code base that all contributions should meet or exceed. If we want to go down the rabbit hole, we can work out the statistics for what portion of Exercism Rust PRs are eventually accepted, and what portion of contributors come back more than once. However, my fundamental premise is that exercises should be idiomatic and complete, precisely because we are teaching students who may not know the idioms yet. They should always be exposed to code which is a good model for what they should eventually produce.

Innovation demands rapid experiment-failure-success cycles.

Failure is an inherent part of this cycle. This comment seems to be mostly about turnaround time, not about the level of quality requested.

It puts the burden of work on individual contributors

I want to lump my response to this with one to the later complaint about #1586: "I spent 2+ hours of my precious free-time on the task."

I also spent an hour on that task. I read through the proposal, and wrote 13 comments on specific sections of code, plus several paragraphs on a strategy for general improvements. Yes, the majority of the work goes to individual contributors, but a thoughtful review also takes work, and is a contribution to the project. My goal is at all times to be welcoming, precise, and when suggesting changes, always to explain both why I want a change and how to fix it. I cannot claim to have a perfect success rate, but that's the metric to which I hold myself.

Approve and merge incomplete work

I feel strongly that this is a bad policy. The main branch should always be in a production-ready state, which in this case, means sufficiently polished to put before students. To do otherwise implies that we must have a process for periodically cleaning things up, and discarding incomplete sections, and quite frankly, I do not have time for that project.

Maintainers SHALL NOT make value judgments on correct patches.

I reject the notion that any code which compiles is correct: our goal here is not to produce compiling code; it is to teach students. A lesson which teaches students the right thing alongside several bad habits is not correct. A lesson which has significant unimplemented sections is not correct.

A correct exercise, and in particular a correct concept exercise, fully teaches one concept and always demonstrates and encourages idiomatic rust. "Fully teaches" there is overloaded: it both completely explains the concept (using #1586 as an example, it encourages both field-access and pattern matching), and it is completely implemented; there's no need to come back and finish the exercise later. This is very intentionally a conservative view, because it is derived from the earlier assertion that the main branch should always be in a production-ready state.

Contributors noting improvements should log them as separate issues

As you say, the bulk of work in #1586 was rejected or rewritten. It does not make sense to me to merge work about which that statement is true. Is it really helpful to merge it, then write an issue saying "by the way, this whole exercise needs what amounts to a rewrite"? It seems more helpful, and more polite, to bring it up to snuff immediately, instead of getting passive-aggressive about it.

[ghost]

Thank you for taking the time to respond with depth @coriolinus. Your counter-arguments sharpen the points I whittle and bring up great ideas. I'll note your points below to ensure I'm understanding correctly and expand with reflection they've prompted.

here seem antithetical to correct software development. At a high level, I do not believe that there is value in sacrificing correctness in favor of iteration speed.

I concede that for some projects, experts should guide software design. In my limited experience as a software designer and from what I see working for other tracks, I am betting on the OM approach. I do feel a little nervous about introducing problematic designs or code.

I find this analogous to what the Rust compiler does: it only accepts programs which are correct according to its metrics, and the borrow checker in particular is more strict than most other widely used languages.

That is a great point. With more resources, I believe we might be able to model that. But I'd counter the relationship between me and a new contributor, me and you, differs substantially from rustc and a lib.rs. This substantive difference sways me to bias the human over the machine.

exercises should be idiomatic and complete, precisely because we are teaching students who may not know the idioms yet

I agree. But I am a student who does not speak idiomatic Rust, especially with program architecture. So, I and contributors in my shoes create another dynamic I believe we need to account for. How do we facilitate novice or intermediate contributions to Rust concept exercises? But I think the OM approach is not mutually exclusive to this premise. Perhaps language tracks face a similar problem to distributed systems. One where "eventual consistency" balances the trade-offs. fancy technical term dressing over 🥗

I also spent an hour on that task. I read through the proposal, and wrote 13 comments on specific sections of code, plus several paragraphs on a strategy for general improvements.

Alas! While I closed the PR I do appreciate the time you spent on it. And the related work you whirled up in the Slack conversation about exploring CSV parsing for learning tuples. I have also learned a lot from your thoughtful reviews. So I did not say "I spent 2+ hours of my precious free-time" as a passive-aggressive critique of your reviews. I am learning how to express my emotions better in the context of work. So I just was venting the frustration and describing my inner experience. That is also a tl;dr about my life right now. too many things to do, too little time

I feel strongly that this is a bad policy. The main branch should always be in a production-ready state, which in this case, means sufficiently polished to put before students.

I agree that for some projects, such an approach creates more problems than it solves. But I don't think that is the expectation for the Rust V3 track. You rightly point out the ambiguity in my question. I am proposing OM and the above points solely for V3 development effort. We could explore what that means for track maintenance long-term, but as you note, there are significant risks we would need to account for.

I reject the notion that any code which compiles is correct: our goal here is not to produce compiling code; it is to teach students. A lesson which teaches students the right thing alongside several bad habits is not correct. A lesson which has significant unimplemented sections is not correct.

Gotcha. I am admittedly taking a lenient view of correctness here. I see us asking different questions here. I am asking "what can I do to expedite progress on the Rust v3 track". I believe you are asking "How can every concept exercise eloquently and sufficiently teach Rust concepts?".

It seems more helpful, and more polite, to bring it up to snuff immediately, instead of getting passive-aggressive about it.

I roundly second communicating clearly to the contributor what needs to be made better. By defining our process openly we set expectations. I hope this would remove any passive-aggressiveness because we would point to it to explain why we are doing things the way we are.
Thank you again for taking the time to analyze and critique these arguments. While some philosophical differences exist here I hope to condense them further in the discussion below. I hope this can deliniate what problem we are trying to solve and how we can build a big tent.

[neenjaw]

As a disclaimer, I am mentioning PR's in this post. In case anyone thinks there is any ill-will, there is none. And I assure you, if there was a problem, I would have discussed it with you directly.

Though I feel like this discussion is among giants -- @efx, @coriolinus, @petertseng you are all immensely gifted in your work and ability so I hope to not offend or cause any concern that my respect to you is in doubt -- I think exercism/elixir may have some voice in this since we have implemented optimistic merging.

Elixir is not a systems language, and so there are some degree of difference as to the rigorousness which is required for daily work with it. I'd like to discuss my experience with Optimistic Merging [OM], but first discuss what the track was facing to better illustrate why it is better now.

Where we were

I remember after out 'kick off meeting' there were about 5-6 elixir-interested collaborators ready to start on the work of basically creating a curriculum. And while I don't think any of us said it, I think we were all overwhelmed by the scope of this because it is one thing to write a practice problem, but quite another to start from scratch and create exercises that build from the start.

So we started with the process of concept discovery. And I think rather than coming up with an exhaustive list, we tried to make a perfect list. If you look back I think we have a silly number of PRs just to make this basic list of ideas to make exercises from. That was exhausting and because of that and COVID and life, that was probably the point where we lost most of our momentum.

The problem compounds

I distinctly remember starting the basics exercise, which is a blatant fork of the csharp/ruby versions. And if you look at #1118, 5 reviewers. You could make an argument that, "That @neenjaw guy put together a real problematic PR and it needed it" (and you may be right as this isn't what I do professionally at the moment) but it was hard to get feedback, bring it up to that level then get another review which required some degree of un-work then re-work.

I had a similar experience as @efx when creating my second attempt at an exercise with numbers #909. I had an exercise which worked, but didn't feel right with subsequent reviews to merge it. I don't really know if it would have worked or not looking back with hindsight, but I closed it and moved on.

Where I was

I was reasonably discouraged at this point for a few reasons:

• I didn't feel like I was doing good work
• I didn't see any momentum with Elixir
• I didn't see the people who were previously interested making PR's
  • And to be clear, I didn't expect it. All or most had expressed IRL proiorities that took precedence in conversation. What I am saying is that it was lonely.
• Elixir wasn't recruiting

What changed

• We implemented OM:
  • First pass on draft PR exercises was a very minimal subset of files
    • Minimize time spent making it
    • Minimize time spent reviewing it
  • We made a commitment to accept imperfection if it got the ball rolling and wasn't incorrect. (I am not talking about program correctness, I am talking about teaching anti-patterns).
  • We decided to approve PRs that were
• I created 5-6 issues with actionable steps for concept exercises to hopefully attract people with ability (at any level) to contribute quickly in a direction which would produce good quality code and momentum.

Summary

I really like where the elixir track is right now. We have 22 exercises, and average ~4/wk, We might soon even have a third contributor soon and I'm hoping that our practice with OM between me @ErikSchierboom and @angelikatyborska will result in more people contributing as they are able. Many hands make light work.

I think a point of @efx's was misinterpreted here though. The idea of merging 'good PRs' and using issues to flag actionable items to make 'great PRs'. I don't think this is being passive aggressive nor accepting wrong work.

No one (other than @iHiD and @ErikSchierboom ) works on exercism professionally (afaik) and we are all here because it is stimulating, challenging, fun. And I will spend my free time growing my skills or contributing my time to places where I feel valued, encouraged, and wanted. And I think an OM approach reinforces positivity, momentum, and progress.

A rolling stone gathers no moss.

[workingjubilee]

So, the "PRs should be merged if they improve things even if they aren't perfect" doctrine described in the Google Standard seems like a fair baseline, does it not? I think correctness is deeply valuable or I would never have touched Rust, but I also think that most software follows an iterative process... what we want to do is to grow in the correct phases. It would really suck if work is merged but isn't done, but also it isn't useful unless it's done, because interruptions in people's lives do happen (hi!) and if something is merged prematurely it risks a lot of confusing impressions. So what we have is more of a disagreement over what an improvement looks like.

Sometimes work should in fact be thrown back, no matter how much we like the author or want to keep them or the work in question. I believe articulating our current implicit strategy more would make our merging strategy more optimistic merely by force of specification and bringing internal scrutiny, and that making a radical change in merging strategy might be too drastic if we haven't sat down and examined for a slightly longer few seconds what our current strategy is and what its benefits are.

[ghost]

Thank you again to everyone for chiming in. I'll summarize my understanding of the options for a way forward.

1. Keep our review process as is; status quo
2. Document our current process. Then, establish by consensus what needs improving. Where viewpoints disagree, delineate disagreements for resolution by voting. For all standards, whether unanimous or majority voted, specify in our track contributing document using the Google Standard as our guide.
3. Adopt OM as I proposed above and document our interpretation of it.

@coriolinus @workingjubilee does this summary seem fair? If so can we vote on which option we prefer? ❤️ for 1. 🚀 for 2, 🎉 three.

[workingjubilee]

Uh, I'm confused. I don't have whatever feature in the GH UI would be required to do that aside from posting a reply.

[iHiD]

(You can leave a comment with the relevant emoji in (🌵), but yes, it seems there are limited "responses" here)

[ghost]

@workingjubilee thank you for finding my bug! I updated the post to reflect voting choices.

[seanchen1991]

To be completely transparent, I would opt for option 3 if the choice were mine alone. Ultimately the idea of keeping momentum going in as frictionless a manner as possible sounds appealing to me. However, I would be loathe to alienate @coriolinus as he's easily the most experienced Rust maintainer; I've learned a lot from his reviews and feedback from my (short) time here. So by default I vote 🚀

[lewisclement]

It's a bit much to take in in a short amount of time, I do have to say that in principle I agree with @coriolinus when it comes to optimistic merging. Sounds to me that you'd be implementing a "developer-focused" development strategy, rather than "product-focused".

But I do get that you'll need momentum to get things done, and just like a start-up is more flexible on policies and customer interaction, we probably should too. At least until there's a smoother on-boarding protocol for new maintainers.

[ghost]

@exercism/rust hello! I've summarized a few ways forward: #1725 (comment). Please react with an emoji to vote or leave a comment with the number you prefer (1, 2, or 3). Thank you!