Just so it’s shared here as well, I’ve revived the old template I had lying around in a PR. Please do try it out on my fork (as described in the PR) and leave feedback!
3 Likes
IME that’s handled by a new function and giving the old one a deprecation warning, but it’s not something that anyone wants to be routine, so it’s important to weigh what goes into promises. That said, we do also document things as being experimental or unstable, so documentation can also warn people of things that can break.
I do wonder if we would benefit from some automation around detecting first PRs.
Maybe in the form of a bot that cross-posts their PR to a slack channel, and/or that posts a “welcome, thanks for your first PR” message.
For a couple of reasons.
- such a message could also help set expectations for what will happen next
- getting a fast first review, sets a good impression and once people have had their first PR or two through they kind of understand how it goes so are likely more patient
- Reviewers knowing this is someones first PR might be extra careful to be kind.
8 Likes
I do agree with that, but I also think that for such a “what will happen next” message to be effective, we need to be clear about what actually happens next. As far as I’m aware, there simply isn’t a process for how PRs are reviewed, if they’re not already related to some work that some core developers are already doing & talking about off-repo. So at the moment, what would such a message actually be?
For me at least, this subjective feeling that there is no process for PR reviews (as well as quite serious bugs in both exported functions as well as documentation not getting fixed) is slowly leading to the impression that Julia is less being maintained, and more being actively hacked on, to put out whatever fire is currently perceived to be burning…
Something like:
Congratulations on your first julia PR!
From here the process will proceed roughly as follows.
One or more people will review the PR, and will likely have suggestions for how can be improved, and discussions as to if the change should be merged or not. You will change the PR, and the process will continue again. This will continue until it ceases in the mode of fixed point iteration. Finally someone with merge rights will merge the PR.
If your PR seems to be not getting responses, it is acceptable to bump it after about 10 days (you can do this by posting on it a message, e.g. “bump”). We try to keep everything moving, but sometimes things do slip off the radar.
If your PR is a new feature, or in some way contentious, it will get the Needs Triage label applied, and it will be discussed as the next triage meeting (time permitting). See the calendar for when that is.
Triage will make the final call as to if a PR should (philosphically) be merged or not.
Everyone is welcome at the triage meetings, similarly everyone is welcome to review, you don’t need special permission.
3 Likes
Apologies to be blunt, but that sounds incredibly like “Your PR will not meet our extremely high standards”, which is subjectively a gross clash with the reality of how PRs to the repo actually happen. There are no high standards to get a PR merged, it’s mostly persistence & bumping that eventually gets PRs merged (if noone raises an objection - and even minor objections can result in huge delays in that process, due to a lack of follow up). Worse, if some change seems incredibly minor (without pointing fingers, e.g. here) there is usually no consideration for the bigger picture - to me, that indicates a lack of consistent standard being applied to PRs. Fixing that would do more than having a Bot post some generic “open source works on iterated improvement” message 
1 Like
It’s not meant to.
I like your proposal @oxinabox. My only concern is how to keep any promises we make. Since we (like most projects) depend on volunteer labor, can we really pledge the time of volunteers? Heck, my experience of reporting bugs in commercial software tools does not leave me entirely optimistic that a meaningful response can be guaranteed even when a company pays people whose only job is to provide customer service.
2 Likes
Personally, I’d prefer a wording more like this, reducing combativeness of the response.
Thank you for creating a PR to Julia!
Before this PR can be merged, we’ll have to check that it’s in a mergeable state, as well as
whether the change is desirable from a content perspective (this is usually more work for new features than for documentation changes). A reviewer will then suggest changes, if necessary, to help get the PR over the finish line. Because the Julia repository is run primarily by volunteers, this can take some time - please don’t hesitate to bump your PR after ~a week, if noone got to reviewing it yet or you’re missing some follow up information!Once a PR is ready to be merged, it’s usually merged within a short period of time. If a PR is inconclusive/may need a larger decision, it’s labelled
triageand discussed on the next triage call every two weeks (see the community calendar for the next date).Thank you again for contributing to Julia!
but as mentioned above, I feel like this only addresses a symptom, not the cause.
8 Likes
Since we’re touching at the other side of the coin on how to make the review process more pleasant/less frustrating for committers and maintainers, what ideas could be explored here? Has the Julia project ever considered using GitHub’s code owners functionality or some similar mechanism like area-specific teams (e.g. a hypothetical @JuliaLang/LinearAlgebra) so that more eyes/hands can share the task of seeing and triaging PRs? I’ve noticed on occasion that issues and PRs may sit stagnant because either nobody knows who would be suited to review them, or someone is pinged but doesn’t materialize (likely because notifications are buried). I’m not sure going full on bureaucracy like some bigger projects have done is a good idea, but being able to remove the single point of failure that is one person checking every one of their outstanding GH notifications seems achievable.
5 Likes
Besides what’s been brought up in this thread, there’s also the possibility of getting GPT-4 or PaLM-2 to do a first-pass review. That should be enough to check for most mistakes, prioritize as “mostly ready/not ready,” and make sure that documentation is up to par–I’ve found GPT-4 to be very good at writing docs, in particular.
I wouldn’t trust GPT to do that, no. GPT has no clue about bigger picture software design/maintenance, especially when it comes to Julia content.
14 Likes
Not on its own, clearly, but I don’t think there’s any harm in adding it as a first check.
Even then - this is surely going to add a lot of noise, with how inconsistent it getting things correct actually is.
2 Likes
I think it is irksom and annoying, to be judged by a stocatic bot (vs a linter which i know how to please).
Having experienced this on other repos, I hated it.
And given the various ethical issues around LLMs, I can see it turning people off.
16 Likes
Bumping these GitHub bots as a way to help with reviewer selection and prioritization. There are likely more out there
4 Likes
Where should Julia feature requests go? - #19 by mbauman has a great summary of how other big and successful projects in this space manage their issues. Is anyone aware of a similar analysis for PRs/code changes? It would be nice to borrow from an approach that we already know works and run trials/quick experiments to get the ball rolling.
1 Like
I wonder if a merge queue would help with this problem.
3 Likes
We turned it on for about a day a month ago and then turned it off (because people couldn’t agree as to what the default merge strategy should be, as well as some other issues)
The fact that the process for getting a PR merged is “be annoyingly persistent with frequent pings” should probably be documented somehow, because it’s not intuitive, leading to frustration for unaware new contributors. E.g., see this PR:
The PR just adds a doc string for a public symbol to the docs.
NB: the procedure for getting a PR merged is to keep pinging until it gets reviewed and merged.
It doesn’t matter, this is the last time.
Feel free to close this PR
7 Likes