Proposal: Introduce a dev-curfew on open patches

I haven’t seen anything written about it so far, but if you look closely, you can see that there have been massive changes. (Being a developer and coordinator who is used to working in teams certainly helps to identify them :slight_smile: )

It became necessary to have development coordinators with the increased team size. This has led to many positive effects.
On paper the module owners existed for a while, but now they finally started to make use of this structure as well as refining it. My impression was that the decision making process has improved a lot and even significant changes in the UI/UX have become possible. Previously, my impression was that everyone could join the UI/UX discussions, but there was no clear goal and there was no team with the power to actually make decide and there were no processes for this. This still looks like work in progress, as it doesn’t seem to always work, but it surely has improved a lot.
I only mentioned the UI/UX because that is clearly visible for everyone. In the other areas, the module teams have been revived as well. On top of that, they added roles like lead architect and admins to further help the decision making process.
Just knowing what is currently happening and what are the next steps is very valuable in a development team. Also knowing whether everything is on track for the release is very important, such that the priorities can be shifted if necessary. But doing that properly takes a lot of time. https://developer.blender.org/project/board/2/
The coordinators also take care that the developers are working in the bug tracker as needed. Even though it may sound like a logical thing to do, that’s surprisingly difficult to achieve without a coordinator once the team reaches a certain size (in my experience at least).
They also introduced days to clean up the code overall. This only became possible with the increased team and is going to be invaluable in the future.

Without all that, something like a roadmap (Modules Roadmap – June 2020 — Blender Developers Blog) would not have been possible. We have seen things like roadmaps in the past, but those were more like wishlists.

Edit: I wasn’t a fan of the way Blender was developed for quite along time. The funding for the open projects meant that most developers could only be hired for a limited time. Once they got used to work in the team, they had to leave with all their valuable know how. I am glad they massively pushed the development fund and it became successful!

2 Likes

If it is about discussing actual development, the devtalk forum is for sure one of the places to go to.
Regarding everything else you mentioned, let me refer to my previous answer to you where I talk in depth about the value of upfront communication. Do you think in the case(s) you are mentioning pure patch review would solve the overall issue?

Thank you!

I think I finally got an answer to a question I’ve been asking all year.

I haven’t written that much about the actual review process, except that I would suggest that a module team-member is appointed to be responsible for the development of the patch, after the module team has decided to greenlight it. Being responsible just means, being the one who is incharge of the reviews and communication with the contributor until it is ready to be committed, or given up if it turns out the be dead end.

Personally, I think it’s a misunderstanding to call the module heads for “module owners”, I think they should be called “leaders”, in the sense that they are the ones to have the vision and drive the ambition for the module and guide contributors and module members in that direction. They should not be owners in the “house owner” sense, where you do what you want with your home, and buy people to do whatever you need done, which you can’t do yourself.

What worked out well in the UI team was that William took responsibility for that leadership and had visions for the UI, but now he has left Blender development for a new job.

1 Like

All I’m wondering is what happened to this:
image

:thinking:

I think they hired David Drayton (was the UX Lead for Cinema4D at some point), but it’s not clear from meeting notes if he only with project for asset manager stuff or for rest of ui too.

3 Likes

Interesting.
If that’s the guy behind the wonderful C4D UI/UX, then that’s a step in the right direction. But yeah, if he’s only going to work on the asset manager, that’s a waste of brain power, the asset manager is nothing. There are huge UI/UX issues that needs to be addressed.

Imagine having to review a fairly extensive patch that touches different files, approving it, then later discover a bug, but the original author has no obligation to try and fix (although most would). The module owner is left with a piece of contribution which either they add to their plate of things to fix, or revert back and build up apprehension towards the next contribution. Maybe I’m just cynical to think this.

I do like the idea about a patch curfew, but the task could just as well fall within the community developers to “prune” the bad stuff and suggest patches that are feasible. Don’t need module owners for that.

2 Likes

I quite like the idea. It’s not feasible to review every patch, of course, but the point should be to give feedback to developers and encourage contributions. A standardized process will (hopefully) curb frustration and waste no one’s time.

The guidelines should include asking if such a patch would get accepted before coding on blender chat. After the patch has been uploaded a set period until there has to be some feedback provided (not a review but design things (like button placement etc)). I think this stage should also be used to weed out contributions that wouldn’t be likely to merge. Maybe an official discussion thread here would be good for this step.

If feedback is positive / the patch is clear for review, the developers and reviewers could try to agree on a timeline for review and addressing the issues that’ll be found.
If a patch is cleared for review there should be an understanding that the module owners want to merge the patch and that the patch will most likely be merged if all of the concerns in the review get addressed.

After that, when the review is done, it should be merged within idk maybe 2 weeks.

The important part aren’t the deadlines, but the regulated process, to make sure that no contribution is being forgotten, because after all that’s worse for the developer than just being told that the feature is not interesting.

2 Likes

Oh, they could just do what they did with the tracker curfew and get rid of anything that takes more than one day to fix and pretend it doesn’t exist. :wink:

1 Like

Well, the irony is that even a thread like this one with a lot of user and contributor viewpoints adressing this very real problem, is also ignored by the BF devs involved handling patches on d.b.o. In a situation like this, what else is there for people outside of BF to do, other than just give up on trying to contribute to Blender?

1 Like

For example, (I often see cases like this), I have a patch D6755, Brecht accepted it, but this patch was not commited. After approval, I updated (due to changes in master) the patch four times. This also means that it will need to be reviewed again. So this is a double job for reviewers and for me.

What’s most frustrating is that it’s not clear what’s going on. It’s just missing, should I remind you of it? Or maybe it’s waiting for the next release or Bcon phase…

Or, for example, very simple patches (fixes) in one line of code, like D8530 D9026 D9099. It’s very easy to review, there’s no reason for this to wait for months.

And yes, of course, it would be nice to see some quick feedback for each patch, for example, in the form of adding a reviewer or some tag. To make it clear that this patch is taken into account. As far as I can tell, all new patches are viewed, just sometimes without any feedback.

12 Likes

Isn’t the problem just that devs like Brecht are very busy, and they just don’t get around to merging it? Or just simply forget to do it because of all other stuff happening?

It would be nice if there was an easy way to call attention to a ‘forgotten’ patch in a not-too-intrusive way.

In these cases, would it be a good idea to add another reviewer? Someone who maybe has a bit more time? Or just plainly to double the chance of someone getting around to it?

It’s a difficult problem, since the reason for the patches lingering like this is most probably that the dev that needs to do the merging is just too busy, and pestering them about it makes them even more busy.

Still, in my (short and maybe naïve) experience blender is better than most big open source projects in this regard.

Growing pains…

2 Likes

Possible things:
Tracker Curfew - not confirmed but pretty much must be it.

Code Quality Day - I read someday it was part of it but can’t say where. It’s very crucial to developers both old and new but very long process.
In 2020/21 Recruitment blog post (no link since new user) there are DevOps vacancy - it says “currently finalizing agreement with excellent candidate” but he should start working this month IIRC (and was discussing this opportunity way before that post somewhere in the summer).
They currently testing Scrum Agile Methodology practices with Geometry Nodes project.

2 Likes

My experience working in medium to large organisations is that fast reviews and merges are a sign of a highly effective team. This does become much harder when a large portion of the ‘team’ might be first-time contributors, but the better practices and methodologies that can be put in place to streamline the review and merge process, the better experience it is for everyone. It feels really good to submit something to review and get either a green-light, or detailed and specific feedback, and that results ultimately in better software.

PRs which have not been merged within a few days of being submitted for review are very uncommon in my job, and if a PR is months old it has almost certainly been forgotten or abandoned by the reviewers and even the original developer. Although this is an unrealistic expectation to have regarding Blender, having a workflow such as the following might enable people to work on these in a more streamlined approach:

Once submitted for review, there should always be an ‘action point’ in the review. Whether it is ‘let’s merge it’ or ‘please fix up these specific things things’, or even ‘I need to discuss this with others/do more research before making a decision’, in each of these cases, either the developer or the reviewer has a task to do which needs to be completed before the review can progress. It is clear to all whose court the ball is in. If at any point it isn’t clear what the current status the review is in, clarify.

22 Likes

The primary purpose of this forum is to have a place for developers to communicate among each other, as well as asking for feedback from the community.
With exceptions, it isn’t the best place for feature requests. What you are asking is far more than that. I agree that the situation regarding reviews needs to be improved, but in my opinion, this needs to be initiated from the developers.
They need to analyze what kind of cases exist, such that they can figure out what kind of reasonable ways exists to handle those in the future. Are there some guidelines which could make sense or are there too many cases different for that? How can it be coordinated when multiple modules are involved? This will require quite some rethinking and enforcing that from the outside is highly likely to fail from my point of view.
What you are suggesting is patchwork (pun intended) in my opinion, which is necessary at one point, but it doesn’t solve the underlying problem.

1 Like

As far as I can see, this is pretty much what is happening among the core developers. They are likely not often surprised by the PRs they encounter, because they know what the others are working on and for new functionality they have the bigger picture or the plan in mind or can easily ask for clarification.

The issue is external contributions which is a far more difficult topic in my opinion. Do you think PRs from the core developers and from external contributors can be handled in the same way?

In your case there might be an assumption that you have commit rights. Just send a note to Campbell asking for such and see what he says. You don’t get to just commit whatever you want - things are still reviewed the same way - but at least you could do so once accepted. It saves them the time to do so, which does take a few minutes per. A couple of the patches you referenced are without a “reviewer” set which makes them not appear on some searches - just needing “User Interface” as group reviewer.

6 Likes

It is certainly more difficult to achieve alignment within a larger team, and in this case an extremely large team. Alignment is the key issue that I can see here and that’s not a straightforward thing to solve. Some responsibility is on the proposing developer to ensure the general aim of their proposal is aligned with the direction the core developers wish to take the code, which might not be immediately clear to some new contributors.

That being said, once you get to the point of submitting code to be reviewed for merging, I would expect alignment to not be too much of an issue. Can PRs from external contributors be handled in the same way as between core developers? I don’t see why not. There is of course going to be more friction, which makes clear process and communication even more important. Having an action point from each review is a useful tool for core developers and community contributors alike.

7 Likes

One practical tip which could be used to resurface issues like this without such an absolutist approach as a curfew/deadline is to have a ‘stale-bot’ style automation which will notify relevant people of things that might be in limbo. The utility of this is simply to nudge the reviewer or developer to clarify the status and who is waiting on what.

10 Likes