Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recognize our review capacity is limited for non-trivial work #1417

Open
pitdicker opened this issue Feb 7, 2024 · 7 comments
Open

Recognize our review capacity is limited for non-trivial work #1417

pitdicker opened this issue Feb 7, 2024 · 7 comments

Comments

@pitdicker
Copy link
Collaborator

pitdicker commented Feb 7, 2024

This issue is not meant to complain, but to discuss a problem that in my opinion we have with our process.

Doing trivial changes is going great. Over the past year more than 150 PRs by me have been merged. Those usually only took me a couple of hours or less to write.

Doing non-trivial changes is very, very hard.

Example: The work to fix DateTime out-of-range panics.

The initial PR that did 90% of the work was written in my spare time over ca. one a week (#1048). It was split in 8 different PRs (#1310, #1313, #1069, #1312, #1070, #1317, #1071 and #1333). Most landed over the course of half a year, with one still open.

Example: The work to convert the API to work in const context

This has just been completed! It took six PRs (#1043, #1080, #1205, #1286, #1337 and #1400). The total amount of work it was could have easily been done in two weeks, but it took 9 months.

Example: Fixing our offset parsing support

My work on this was finished around the end of May 2023. One part has been completed at the end of June (#1082), #1145 was merged at the end of august, part 2 (#1083) is still open, and part 3 is ready waiting on the merge of part 2 to become a PR.

Example: New formatting API

This was very difficult work (for me) that took ca. 3 weeks of spare time (#1196). So far none of it has seen a review, including simpler split out work in #1184, #1163 or #1335.

Example: Fixing panics on Windows Local

Work on this was completed in ca one week in April 2023 (#1017, my third PR to chrono). That PR has been all over the place. It was extended to unify part of the Unix and Windows code, to handle parts that later became the 'DateTime out-of-range panics'-work (#1048), further refactorings of the platform code (#1072), further refactorings of the Unix side, optimizations, and scaled back to just Windows.

We could have long shipped it, as the core of the PR remained unchanged. Any improvements could have been later PRs.

Example: work on support for parsing ISO 8601 formats

Work on this was completed over a few days, and has since June lingered in #1143.

Example: work on adding a CalendarDuration

The basic PR to add the type was opened in September (#1290). I have two branches to start doing meaningful operations on it. It is going to be a couple of weeks worth of work more to finish adding this type, spread over a whole bunch of PRs that have to depend on each other.

Example: adding a niche to NaiveDate

Work started in #1207 in July. That lead to a refactoring of the internals module in #1212 in August. Part of the work was split of to another branch for a follow-up PR. The refactoring is now blocking other work in the module.

Example: convert our API to return Results.

This is again going to be work that will require many PRs that build upon each other. Because of how much it would conflict with the work to make our methods const starting on it was delayed from June last year until now.

It can probably be finished in two months of work spread over ca. 15 PRs. But at the rate previous non-trivial work has gone it may well turn out to take more than a year, maybe even two.

The work to get a 0.5 release

If we want to make the API changes for 0.5 that have proven to be desirable in our issue tracker, that can very well be completed in less than a year of work for me, maybe even half a year. At the current rate it is going to take 4 times longer or more.

The process introduces issues

I like the extra quality reviews bring. However the current process introduces more issues than it catches. If a PR has to rebase 10+ times that is likely to introduce issues. After a couple of months the details are paged out for me. If a lot of code around a PR changes, some of the changes may be subtle enough to slip through while I would have caught them while writing the code. Review is not catching those.

The process is draining

Curating open PRs by rebasing them to keep them in a merge-able state is easily taking ca. ⅓rd of my time. This can be spend better. To friendly keep asking for reviews while recognizing that all we do is volunteer work is not fun. Seeing all the work that took a lot of effort go to waste by being unmerged, or merged with an inordinate amount of effort from my side is simply draining for the motivation.

Not everything can be reduced to trivial PRs

Some bug fixes or improvements just require a 1000+ lines to change. It can be split in small(ish), self-contained commits. That may take me double the effort, but I am more than willing to do that. But I can only meaningfully test something is fixed with the full set of changes.

Of course a 12-commit PR can be split into 6 PRs that build upon each other. We have done so for some of the work mentioned above. I am willing to keep making a 'master-PR`, provided the split our PRs are going to make progress in a reasonable time frame.

Work starts blocking each other

On average I have 25+ open PRs, and 25 other branches that can't even be opened as PR because they depend on them. I have to keep the state they are in in my mind. How do they depend on each other?

"Didn't I fix this issue before? Why am I touching the same code again? O yes, it is in one of the unmerged branches from a couple of months ago."

At some point it just becomes difficult to find parts of the codebase I can work on that don't cause a massive merge conflict with already open PRs.

How to fix?

I'll leave that to @djc. Hopefully we can relax the review process, as I can't just demand more time from you.

I love working on chrono. But the process to do non-trivial work has already drained my motivation once.

@djc
Copy link
Member

djc commented Feb 7, 2024

I definitely understand that this is frustrating -- and I'm very sorry much of your hard work is not getting the attention that I agree it deserves. I've thought about this quite a bit, so here are some thoughts:

  • Your capacity to make changes outstrips my capacity to review. Unfortunately I don't think this is likely to change -- and I've long since recognized this is the case, but have struggled with how to address this.
  • Due to this, you have a lot of work in progress. A problem on my side is that I manage my open source projects mainly by looking at my incoming GitHub notifications. That means any PR you update (even for a rebase) generates a notification. As such, when you generate 10 notifications in one day I have a hard time picking which one to work on, and, as a result, sometimes to skip chrono work that day because I get frustrated with the incoming amount of stuff. Much of my process is trying to drive down the number of notifications that are waiting for my input so when you (unintentionally, by producting a lot of good work!) make it harder to do this, my motivation to work on chrono gets reduced. I think there's a sort of spiral here where I get demotivated, then you get demotivated by the lack of progress and we both stopped working on chrono for a while. While that's unfortunate, IMO it was also a good thing at least for me as it allowed me to get on top of other things and see that chrono is basically in a good place.
  • You mention having trouble keeping all the state in your mind -- now imagine doing the same thing across PRs from 6-8 different projects -- that's what it's like for me. This is an important reason why it's harder to get non-trivial work merged, because I have to find time/energy to dig into the non-trivial work, page in the context and try to make good decisions.
  • Of all the projects I work on, chrono often doesn't get top priority for me. Obviously I am not (substantially) paid to work on it, and it basically works well for most users. This makes (especially) design-focused work for 0.5 less urgent, and so in terms of priorities I have decided to not give it as much attention as other things.
  • This issue focuses on the negative, but I would argue that in our collaboration we've also got a lot of good work done. Since May of last year, we got 9 point releases out and managed to make chrono more robust and got rid of the time dependency without any major issues. So please remember this, too.
  • For some of the issues you raised (the formatting API is a big one for me), my problem is that I don't find it obvious what the right design is.

I don't feel like the right solution here is to review less -- I feel it as a responsibility to carefully review code going into chrono and I don't (yet) trust you to maintain it without me. I think it could help to carefully and explicitly prioritize what we are both motivated to work on and to limit ongoing work to fewer things at a time. It feels bad that I'm constraining your time/energy to improve chrono but I haven't been able to come up with a better solution.

@tertsdiepraam
Copy link

Hi! I was just passing by considering to contribute to chrono and wondering if I can help out here.

  • I think this situation also has an effect on other contributors, based only on the GitHub activity, it's hard to know what I could contribute, since it might already have been done by @pitdicker. I was planning to just check out the 0.5.x branch and see what I could do, but that doesn't work in this situation, because there are big unmerged refactors. It's not a big deal, but I think it's an aspect to consider as well.
  • To me (as an outsider), it sounds like prioritization would be the key here. If you give a few PRs/issues priority, the context switching can be reduced to a minimum for both PR authors and maintainers and PRs can be reviewed and merged at a higher rate. The rebasing would also be kept to a minimum.
  • I'm new to this repository, but I'd be happy to help with reviewing and discussing a couple of the open PRs, so that when @djc gets to reviewing them they have already gone through a review round. Of course, I'm not suggesting to give me merge rights, I would just post the reviews. Just tell me which PRs have priority and I'll take a look.

And I second @djc's comment about chrono being in a pretty good state. We're happily using 0.4 in uutils/coreutils and the 0.5 branch is looking better than ever!

@pitdicker
Copy link
Collaborator Author

pitdicker commented Feb 7, 2024

A problem on my side is that I manage my open source projects mainly by looking at my incoming GitHub notifications. … As such, when you generate 10 notifications in one day I have a hard time picking which one to work on, and, as a result, sometimes to skip chrono work that day because I get frustrated with the incoming amount of stuff.

I definitely recognize that! Crossing off emails, but getting overwhelmed by the number of them that come in for a project and then parking them all for a while 😄.

  • You mention having trouble keeping all the state in your mind -- now imagine doing the same thing across PRs from 6-8 different projects -- that's what it's like for me. This is an important reason why it's harder to get non-trivial work merged, because I have to find time/energy to dig into the non-trivial work, page in the context and try to make good decisions.

I have said it before, but really mean it: I have a lot of respect for everything you do! You are a responsible maintainer and have taken on much more responsibility that I would want (for open-source work that is).

This issue focuses on the negative, but I would argue that in our collaboration we've also got a lot of good work done. Since May of last year, we got 9 point releases out and managed to make chrono more robust and got rid of the time dependency without any major issues. So please remember this, too.

Completely agree. And I am happy with that.
This issue may sound negative, sorry. But I did want to discuss what I view as a problem.

I don't feel like the right solution here is to review less -- I feel it as a responsibility to carefully review code going into chrono and I don't (yet) trust you to maintain it without me.

You're reviews have definitely help me to improve my coding, and I appreciate them.
It would be great I we can have a solution that doesn't burden you more, while allowing better progress.
And I don't want too much work to go to waste.

I think it could help to carefully and explicitly prioritize what we are both motivated to work on and to limit ongoing work to fewer things at a time.

Sorry, can't agree there. The last time we discussed this you also suggested we work on shrinking the API and work towards 0.5 (including the fallibility and formatting work) instead of 0.4.

Is there any middle ground we can find?
For example that I get more freedom for the 0.5 branch, and you'll nuke it after a year if I haven't earned enough trust by then?

Edit: Another option is suggested at some point is that we discuss some PRs on the desirability / risks / API changes only.

@djc
Copy link
Member

djc commented Feb 7, 2024

Sorry, can't agree there. The last time we discussed this you also suggested we work on shrinking the API and work towards 0.5 (including the fallibility and formatting work) instead of 0.4.

Why can't you agree?

Is there any middle ground we can find? For example that I get more freedom for the 0.5 branch, and you'll nuke it after a year if I haven't earned enough trust by then?

What freedom do you want? If you spend your time on the 0.5 branch without me reviewing I will feel obligated to review all of your changes at some point before it releases, which is unlikely to ever happen.

If you prefer, you can fork chrono and start a separate project with a chrono-inspired API that conforms to your vision for what chrono 0.5 should be. But that project will no longer be chrono.

Edit: Another option is suggested at some point is that we discuss some PRs on the desirability / risks / API changes only.

I feel a responsibility to review every line of code/comment changed in the core library. This is not because I like it (I definitely get more stress from maintaining chrono than I derive satisfaction from it) but because the previous maintainer entrusted me with this crate and it has lots of people depending on it. As such, I don't think this will change until/unless a maintainer comes along who I trust to apply a similar level of scrutinee.

My obligation in maintaining chrono is towards chrono's users first, and towards contributors second. I'm sorry if that doesn't work for you (and some chrono users might get disappointed if you no longer want to work this way -- though I'm afraid only a tiny fraction of users pays attention) but I don't feel like I have many degrees of freedom here.

@pitdicker
Copy link
Collaborator Author

pitdicker commented Feb 8, 2024

If you prefer, you can fork chrono and start a separate project with a chrono-inspired API that conforms to your vision for what chrono 0.5 should be. But that project will no longer be chrono.

Believe me, I only want to either help out with chrono or switch to another hobby. Introducing a fork or a third date and time library into the ecosystem is not for me.

What I am looking for is not really freedom. Let's phrase this issue differently:
Converting the API to return Results is going to be a big undertaking, much larger than past efforts that took me multiple weeks. I started this issue to openly discuss my worries.

  • Can I be sure this project can be driven to completion (without many weeks / months between PRs that depend on each other)?
  • I don't want to burden you more.

@djc
Copy link
Member

djc commented Feb 8, 2024

It will be driven to completion, but I can't give any guarantees how much time it takes. I am motivated to get this work done, but it will need to be done in small increments (#1410 is a good start).

@pitdicker
Copy link
Collaborator Author

I can't give any guarantees how much time it takes.

Same for me 😄. I'll split it into the smallest units that still make sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants