Better organisation of reviews

It might change the result, or it might not - I'm not sure it's fair to generalise about that. I've participated in reviews which seem to be going one way until somebody chimes in much later on with some domain-specific expertise or use-cases.

Personally, I'm finding it difficult to keep up, and that's why I want to revisit the question of whether it is really necessary for reviews to be so short. I feel that the onus should be on justifying why reviews are so short, rather than the opposite. With such short reviews, even the smallest of things can materially impact how much time somebody has to read, consider, and express their thoughts. Take the review about property wrappers as function arguments - something like that is likely to have a very big impact on the language, and it feels rather unprofessional that circumstances like a sick child would significantly limit how much somebody can participate in that review. It is mostly a question of professionalism.

As for the delay: the part of this that matters most (IMO) is whether the review time causes tangible delays in shipping the feature to developers in official releases. But official releases don't just spring up and surprise us: they are planned and staged, so I don't think this is actually going to delay when things ship. I can only remember one very time-sensitive proposal, and of course I expect the core team to reserve the right to make those kinds of exceptions as needed.

2 Likes

Proposing and implementing new features in a major programming language is not a small thing. It will affect many thousands of developers who use the language on a daily basis. I also feel that if somebody goes through all of that work, it's only respectful that it be given enough time for review.

Or to put it another way - I want to be able to give them more than my "gut feeling". But with time constraints being what they are, that's all I can afford to give them. I think they (and the language as a whole) deserves better than that, even if reviews are only advisory comments for the core team.

Maybe I'm just overworked (true) and nobody else feels this way, but I wanted to put it out there because I suspect it's not just me.

4 Likes

My point was that the engineers implementing these changes may also be overworked and have deadlines before the holidays and your proposal makes no acknowledgement of that fact. I don't think your proposal would help and would make the evolution process take even longer, which is not something the people working on the language would appreciate.

1 Like

Thanks for starting this discussion, @Karl—I've had similar feelings at times of being unable to devote the time to proposal review that I felt was deserved.

My subjective experience has been that there tend to be 'waves' of proposal reviews: periods of time where several proposals are under review at once, followed by periods where there is little-to-no review activity at all. IMO, it would be a better experience if these waves were avoided and overlapping review periods minimized.

Based on @Chris_Lattner3's post above, I'm struck by the implication that scheduling of individual proposals seems to be entirely delegated to each RM as they see fit (please correct me if I've misunderstood). Perhaps it would be better for review scheduling to be taken up more as a collective effort. I realize that this would put a larger coordination burden on the Core Team, but I think even a policy of something like "at least one week between the start times of consecutive reviews" would alleviate much of the frenetic feeling that sometimes arises.

Aside: a 'nice-to-have' would be for Swift Evolution to have some sort of 'calendar' view for the active/scheduled proposals, in addition to the current list view. :slightly_smiling_face:

Edit: clarified the suggested policy; "at least one week between consecutive reviews" -> "at least one week between the start times of consecutive reviews"

5 Likes

Sure, I guess many of the Apple engineers are trying to get these things closed before the new year. But so is everyone else! I don't see how it justifies a lightning-quick review.

My proposal wouldn't just make the review period longer (although it would do that); it would also make it more regular. Everybody would know when that month's round of reviews is going to show up, so you wouldn't have to stay glued to the forums for fear of missing something with a high impact.

2 Likes

I agree with Doug that I don’t think we’d actually get more feedback from longer review periods—we’d just slow down development. But that doesn’t mean Karl is wrong to feel like the review load is a little high right now. I noticed this too—it’s not so much that I couldn’t keep up, but it’s my job to keep up, and that’s not true for many Evolution participants.

@Karl I wonder: To what extent do you feel like this is too much because the review periods are too short, vs. because there are so many reviews starting at once? Three new proposals + a re-review on a single day does seem a little excessive to me, but I don’t think I’d feel that way if they were staggered over a couple of weeks.

If the problem is just too many reviews running at once, I could see us putting in place guidelines like:

  • Reviews should start on a Monday (probably actually “first workday of the week in the US”).
  • Limited scope re-reviews should be at least 7 days.
  • Other reviews should be at least 14 days.
  • No more than two limited scope re-reviews and two other reviews should start in each week.
  • These are just guidelines; the core team is expected to deviate when there is time pressure or another special circumstance (e.g. more than two related proposals which should be reviewed together).

Lots of room to argue about the specifics (maybe reviews should start on a Friday and end on a Monday so there’s an extra weekend in there?), but you get the idea.

7 Likes

Won't presume to speak for @Karl, but personally I feel that the ~concurrent~ reviews are the bigger issue. This period feels particularly crammed because we also have the numerous concurrency proposals undergoing their pitch phase. They may not be formal reviews, but they still contribute (for myself) to the feeling of a large amount of high-stakes activity.

Without getting into the weeds of the specifics too much, I would welcome a set of guidelines of this sort. I'd also maybe throw in something about a scheduling announcement that will go out 7 (or 5, or 3) days before a review commences.

2 Likes

It's the combination. I wouldn't necessarily say that short reviews are fine if there are less concurrent reviews or vice versa (as @tkrajacic pointed out, the effective, available time somebody has doesn't scale like that).

Your ideas do seem like they would lighten the load, though, so personally I would appreciate that. Every little helps.

I'd also be interested to hear about people working in larger teams of Swift developers - especially these days, perhaps it would be easier for them to schedule internal group discussions if reviews were slightly longer and happened in more regular intervals.

One thing is that with such short reviews, it's easy to miss them if you're on holiday or something. It's not unusual for people to take 7-14 day holidays.

Now, a lot of developers are used to working after-hours, at weekends, and checking the forums while technically "on holiday" probably isn't too uncommon. But that's not really a great thing - it's important for mental health and to prevent burnout that people actually get time to switch off.

I really feel that if we lengthened the review periods across the board, that it would be a lot easier to do that.

2 Likes

I don't think extending the standard review time past 14 days is feasible for scheduling reasons. A proposal scheduled now for 14 days would miss the branch date for the Swift 5.4 Spring release, even if the proposal was universally accepted as-is and fully implemented. If you start lining up month-long reviews (+ expected re-reviews) for, say, the dependency chain of Swift Concurrency proposals you'll almost certainly push yourself past the next branch date. And longer review periods makes for more overlapping reviews, which means more things to track at once. We trade one problem for another.

I maintain that the primary issue is the surprise. Right now, it's plausible that a two-week holiday could completely straddle a review period and you wouldn't know until you saw that the review was done. If we schedule reviews in advance and it conflicts with your holiday, you can effectively time-shift: write your review in advance, decide to check in once to post it, etc.

For example, with the set of reviews that just launched, the proposals were effectively ready before the U.S. Thanksgiving holiday. Had the Core Team posted the schedule before that holiday, there would be no surprise early this week, and folks with an interest could have chosen to read the proposals over the holiday (or not) as appropriate. Anyone who wanted to schedule a discussion with their colleagues would have been able to do so during the review period.

I checked in with the Core Team, and we're going to try to "pre-schedule" some of the Concurrency-related proposals to try to help here. These are the largest proposals going through the process in the near future, and there are several of them with dependencies. We're mindful that we need to start the review process, that some parts of Concurrency seem to be mostly settled while others are much more in flux, and that we have another holiday season coming up during which there will be no reviews. Here's what we're looking at:

  • Schedule async/await for December 8...22

  • Schedule "Objective-C Interoperability with Concurrency" for December 10...22

  • Schedule "Structured Concurrency" for January 5...19.

  • Schedule" Async Sequences" for January 5...19.

    Doug

10 Likes

Looking at the cutoff dates for the last few releases:

So we do tend to have quite a bit of time between releases. I think there is probably some leeway to extend reviews within that time (maybe extending all deadlines by 7 days - so 14 days for re-reviews, 21 days for longer reviews).

Perhaps, but with the schedule you posted (very informative BTW, thanks!), we're apparently looking at 5 concurrent reviews (!) come December 10. So shorter review periods do not seem to be leading to less overlapping reviews today, and the overall load is still very high.

Most of those 5 will be at the tail end and are unlikely to be active. Moreover, today we are crunched between two holiday periods. If we had 21-day reviews and wanted to avoid additional overlap now, we'd schedule those after the holiday break---and then either have 4 concurrently-running concurrency reviews in January (most of which are large, so that's not great) or be staging them out and not be able to start the second round until the end of January. There's also a third round (e.g., actors, then global actors) that would follow, which would then only start in middle or late February, and that historical "March 18th" branch date you cited becomes a very, very real constraint.

We would very much like to accommodate people's schedules and improve the review process, but there are many constraints on this. We'll see if pre-scheduling reviews and having slightly longer review periods can help.

Doug

2 Likes

I don't think that extending the review time is required in general, and I'm happy for RMs to decide their own timeline. I don't want to see a uniform review period of an entire month, because that is clearly overkill for almost all proposals. It's always easier to argue that more time is better (e.g. it allows more time for consideration and more people to participate) but there are downsides too (e.g. it can waste a lot of people's time and it makes the whole system of making changes less efficient). I have personally seen more reviews that I wish had closed earlier than later, because there is a lot of active discussion but no new substantive arguments.

From what you've written here it sounds like you're personally feeling a lot of time pressure at the moment. There have been times when I haven't been able to read the forums for weeks, or months, also. I think you have to accept that you won't always be able to participate in reviews, and trust that the people who do participate and the core team will make good decisions without you.

This would be the worst case for me, because I don't have much time during the week, but it just makes the point that you can never please everyone.

I disagree with this sentiment.

Sometimes one review spots something that no one else sees, and turns the tide.

Sure, it's possible, but that's life. I don't think it's reasonable or healthy for anyone to think “I have to participate in every review or they might make a major mistake or wrong decision without me”. We all know that participating in a review is no guarantee that you'll get the outcome you want, anyway.

1 Like

I think you might have misunderstood - I'm certainly not saying that and don't think that.

We have an evolution process so that developers who use Swift can help shape its future. Those who choose to participate do so because they are motivated to create a better language through collaboration and sharing ideas/experiences. If time limits are preventing them from fully participating or causing undue stress, I don't see the harm in asking whether those limits can be relaxed a bit.

Anyway, I think I've made the point I wanted to make. At least some members of the core team have seen it, so hopefully they'll factor it in when scheduling future reviews. Pre-scheduling is a welcome step: I learned that the concurrency reviews are coming a lot sooner than I expected. That helps me plan my time a bit better, and I appreciate that.

2 Likes

I wasn't talking about you there, just replying to @wowbagger's post which implied that individual pressure.

Just to clarify, I proposed this timeline when we discussed it at the most recent core team meeting and others found it agreeable, so there was collective action there.

One of the reasons that a bunch of proposals got scheduled all at once is that it was correctly observed that we had a backlog and had to get on it.

I can understand Karl's concern, but there is a balance that needs to be struck here - we have to be aware of the time limitations on the side of the proposal authors and release managers as well. While we should continue to improve things, I don't think we'll ever achieve perfection. I don't think a rigid process is going to be better in practice.

-Chris

4 Likes

Thank you for clarifying on that point, Chris!

Well, those were optimistic ;)

We just scheduled Async Sequences as SE-0298 for January 12...26. Structured Concurrency has not yet converged to the point where it is ready to schedule a review (nor have actors).

Doug

6 Likes