search

apache / arrow-julia

Official Julia implementation of Apache Arrow
https://arrow.apache.org/julia/
Other
285 stars 59 forks source link

Need to improve code review/release process and reduce developer productivity friction #342

Closed quinnj closed 1 year ago

quinnj commented 2 years ago

In https://github.com/apache/arrow-julia/issues/284, I originally raised some concerns about the health and long-term maintainability of the package under the apache organization.

Having let that sit for a while, I'm again raising concerns around how the package is managed. In particular, I have 3 main complaints:

  1. Inability for meaningful contributors to approve pull requests (only Arrow PMC members are able to approve PRs to be merged)
  2. Inability for meaningful contributors to approve new releases (same as above)
  3. Slowness of getting fixes merged and new releases made (combination of requiring Arrow PMC approvals from above 2 and current 72 hour release window)

On point 1, it's unfortunate because only Arrow PMC members (only @kou so far) can approve PRs/releases in a meaningful way, yet these members, no disrespect intended, don't have the skills/context/code abilities to actually evaluate code changes. It would be much more helpful if @jrevels, @omus, @ericphanson, @nickrobinson251, @bkamins, and @baumgold had the necessary permissions to approve pull requests and new releases.

On point 3, the current 72-hour window is really long. Especially when it's idiomatic in Julia packages to merge a single pull request with small fix, and immediately issue a patch release. I think ideally we'd be able to have at least 12 or 24 hour release windows that would make things much more manageable.

Thoughts?

baumgold commented 2 years ago

I wholeheartedly agree. Who do we need to convince in order to make a change?

bkamins commented 2 years ago

I think, for enterprise adoption, there is value of the fact that Arrow.jl is managed under Apache Arrow umbrella. What I write below assumes Arrow PMC wants to stick to its current rules.

So regarding 1. and 2. (PR approval) I think that the practical solution could be:

  1. A proposal to add @quinnj to Arrow PMC.
  2. Then the process of making PRs would require that someone else that @quinnj makes PRs, and @quinnj reviews them. It would be problematic, but the long-term benefit of this would be that we would have a larger pool of Julia developers that know the internals.

Regarding 3. (merging policy) - what @quinnj describes is indeed what is a typical practice in Julia community. However, if Apache PMC wants to keep this rule the good thing is that Julia package manager is flexible enough to work-around this. If user is hit by some bug and it is fixed but not released yet user can install main branch temporarily when waiting for a patch release. This is feasible because Arrow has mostly linear development (so it is not e.g. like DataFrames.jl where we usually add loads of new features in main and patch releases require a separate branch).

Having said that, I think that the starting point for this discussion is a question how Arrow PMC sees the PR approval process given your comments, so that it allows for smooth development while ensuring safety (as this is what I assume Arrow PMC rules are designed around).

pitrou commented 2 years ago
  1. Inability for meaningful contributors to approve pull requests (only Arrow PMC members are able to approve PRs to be merged)

This seems to be misleading since: 1) any committer should be able to merge PRs, not only PMCs; 2) anyone can review PRs, a committer is only needed to do the actual merging.

That said, given that the Arrow Julia community operates entirely separately from other Arrow development communities, and that ASF policies do need seem to suit its preferred workflow, it would IMHO make sense for Arrow Julia to leave the ASF umbrella.

lidavidm commented 2 years ago

I don't think there's many active committers (beyond Jacob), though, so the distinction doesn't help much here. I also don't think just operating separately is an issue, and the challenges here around releases are also faced by other Arrow subprojects (Go, Rust). Finally, we just got arrow-julia back under the ASF umbrella; I don't think we should see these problems as a sign to just throw in the towel immediately.

kou commented 2 years ago
  1. Inability for meaningful contributors to approve pull requests (only Arrow PMC members are able to approve PRs to be merged)

Apache Arrow commiters (including @quinnj) and Apache Arrow PMC members can approve pull requests.

If the approval for pull request is meaningless for us, we can disable the "require 1 approval before merging a pull request" feature by removing https://github.com/apache/arrow-julia/blob/main/.asf.yaml#L41-L43 . It's just added because the original repository has the configuration.

  1. Inability for meaningful contributors to approve new releases (same as above)

Is it for voting a new release? Or is it for merging a pull request to bump version such as https://github.com/apache/arrow-julia/pull/312 ?

If it's for voting, at least Apache Arrow PMC members need to audit license for each release. See also:

Because Apache Arrow PMC is responsible for ensuring that all Apache Arrow artifacts comply with legal affairs policies: https://www.apache.org/dev/pmc.html#legal-policy

This is for legal protection. See also: https://www.apache.org/legal/release-policy.html#why

If it's for merging a pull request, the current Apache Arrow Julia contributors can confirm changes for a new release and comment on a pull request to bump version. See also my answer for 1.

It would be much more helpful if @jrevels, @omus, @ericphanson, @nickrobinson251, @bkamins, and @baumgold had the necessary permissions to approve pull requests and new releases.

OK. I'll start discussions about inviting them as an Apache Arrow committer on private@arrow.apache.org. Could you provide a text to use the discussion for each person? (@quinnj doesn't need to write all texts for them. Others can write them.) See also: https://cwiki.apache.org/confluence/display/ARROW/Inviting+New+Committers+and+PMC+Members#InvitingNewCommittersandPMCMembers-Step1:Startadiscussionthread

On point 3, the current 72-hour window is really long. Especially when it's idiomatic in Julia packages to merge a single pull request with small fix, and immediately issue a patch release. I think ideally we'd be able to have at least 12 or 24 hour release windows that would make things much more manageable.

We can use 24 hours because 72 hours is just "SHOULD": https://www.apache.org/legal/release-policy.html#release-approval (12 hours will be short for considering timezone.) But we still needs at least 3 +1 from Apache Arrow PMC members.

BTW, we already use 24 hours not 72 hours. e.g.:

@quinnj Are you interested in joining Apache Arrow PMC? If you're an Apache Arrow PMC member, we'll get 2 +1 (from you and me) soon. We just need one more +1 for a new release. If one more Apache Arrow PMC members from Apache Arrow Julia community in the future, we'll get 3+ +1 soon.

quinnj commented 2 years ago

If the approval for pull request is meaningless for us, we can disable the "require 1 approval before merging a pull request" feature by removing https://github.com/apache/arrow-julia/blob/main/.asf.yaml#L41-L43 . It's just added because the original repository has the configuration.

Yeah, it would probably be best to disable this for now. I think it's a nice feature on larger codebases, but for Arrow.jl, I think it's just too small at the moment.

Are you interested in joining Apache Arrow PMC? If you're an Apache Arrow PMC member, we'll get 2 +1 (from you and me) soon. We just need one more +1 for a new release. If one more Apache Arrow PMC members from Apache Arrow Julia community in the future, we'll get 3+ +1 soon.

Yes, this would also be helpful.

Could you provide a text to use the discussion for each person?

@jrevels has been a solid contributor by reporting issues, making pull requests, and bringing up larger design discussion for the project as he works on the key data integration of arrow data at Beacon Biosignals.

@omus is a solid contributor that helped a lot in the process of registering the ArrowTypes.jl subpackage and working out CI wrinkles so Arrow.jl and ArrowTypes.jl worked together nicely. He has also contributed to larger package design discussions in meaningful ways.

@ericphanson has been super helpful reporting issues, making fixes, and helping triage issues as they come in to the repo.

@nickrobinson251 has started helping triage issues and has shown interest in working on finishing the C data interface for the Julia implementation. He is strong in design discussions and would help being able to provide fixes for issues as they come in.

@bkamins is a strong contributor across the JuliaData ecosystem. He's a leading member of larger, coordinating projects that affect lots of packages and is the primary DataFrames.jl maintainer. It would be helpful to include him as a committer to help review inter-package PRs in the Arrow.jl repo (like for the recent efforts to coordinate Julia-side metadata for tables).

@baumgold has made a number of large, meaningful pull requests to the project, in addition to helping triage issues and do code reviews on outstanding pull requests.

quinnj commented 2 years ago

If the approval for pull request is meaningless for us, we can disable the "require 1 approval before merging a pull request" feature by removing https://github.com/apache/arrow-julia/blob/main/.asf.yaml#L41-L43

PR up to remove requiring approvals: https://github.com/apache/arrow-julia/pull/343

kou commented 2 years ago

Thanks.

I've merged #343. Now, we don't require any approval to merge a pull request.

I've started discussions for Apache Arrow committers and Apache Arrow PMC member on private@arrow.apache.org. Please wait for a while. I hope that at least some of you join Apache Arrow commmiters/PMC.

quinnj commented 2 years ago

Thanks @kou

quinnj commented 1 year ago

I think the developement/release process has improved significantly; many thanks to @kou for all the help here.