Git Product home page Git Product logo

Comments (5)

chrishavlin avatar chrishavlin commented on September 21, 2024 1

Thanks for starting this discussion @munkm !

I'm in favor of broadening the scope of PRs that require just 1 vote, particularly any infrastructure/maintenance PRs. I also agree with Matt that we need to somehow acknowledge the nebulous nature of any categories we define.

In terms of new features, where do new frontends fall? They can take quite a while to get through and since they're relatively self contained I could see a benefit to relaxing the approval requirement from 2 to 1 if some amount of time has passed. I'd still lean towards requiring 2 approvals since with the amount of new code it's helpful to have more eyes on it, but I thought it was worth bringing up.

from yt.

neutrinoceros avatar neutrinoceros commented on September 21, 2024

I generally agree with you here. I would like to respond to a couple specific points

new features probably should have 2 approving reviews to make sure that the feature is consistent with community needs/expectations.

While I agree that 2 reviews is generally better than 1, and that it'd be best to have as many reviewers as possible on features is ideal, I would argue that timing should also be considered. In some cases, it takes a very long time (several months, or even years) to get a single approval, and this can be crushing (when not a straight deal breaker) to the contributor. Not only does nobody expects to be in the queue for this long, keeping a PR alive also requires extra care and skills that I don't think we want to impose as another entry barrier.
This is, understandably, mostly due to a lack of dedicated time for reviewers to get through the backlog. In the absence of clear objection, like changes requested, or another maintainer clearly expressing that they'd like more time to review a change, I think it could be good to allow even new features to get in with a single approval after some to-be-discussed period of time.

But if we are just following NEP 29 then we have already established a drop schedule for most upstream packages, so is 2 approving reviews really necessary?

We may need more discussion on this. I know you co-signed NEP 29 on behalf of the project, and as the person taking care of enforcing python version dropping for the last 3 years, I'm absolutely in favour of following it. However, this is rarely discussed and I'm not sure every maintainer is aware that this document exists, or agrees to follow it. It also is now superseded by SPEC 0. Note that we are currently supporting a larger version range than this specification recommends : at the time I'm writing, we just dropped Python 3.8, and SPEC 0 recommends that we drop 3.9 in two weeks; we support numpy 1.19, while SPEC 0 already recommends dropping 1.21.

from yt.

munkm avatar munkm commented on September 21, 2024

In some cases, it takes a very long time (several months, or even years) to get a single approval, and this can be crushing (when not a straight deal breaker) to the contributor.

I am curious what examples exist for this past yt-4.0 or after the examples where the entire codebase was refactored and contributors were asked to wait until those PRs went in. The other example I can think of are PRs that were modifying important aspects of our community procedures. In my experience, the PRs that take a long time to go in are the ones that should have 2 reviews, as they either change a lot of the codebase or they require substantial community discussion because they change existing processes. Having 2 community members review (and ensure everything is satisfied) helps reduce the burden of a single reviewer catching everything.

I know you co-signed NEP 29 on behalf of the project

I didn't co-sign it on behalf of the project. I co-authored (and SPEC 0) it as a scientific python community member.

However, this is rarely discussed and I'm not sure every maintainer is aware that this document exists, or agrees to follow it.

Our community discussion about it happened after the NEP was published in #2307 . Do you have suggestions of a different venue where this type of conversation should happen so that the community knows we are following it?

FWIW the goal of NEP29/SPEC0 is not to have explicit version support drops, but rather that maintainers shouldn't be burdened to maintain support for those versions. There's no issue with us continuing to have Python 3.9 or numpy 1.20; our users should just expect that 3.9 support or Numpy < 1.21 is unstable and YMMV.

from yt.

neutrinoceros avatar neutrinoceros commented on September 21, 2024

... examples where the entire codebase was refactored and contributors were asked to wait until those PRs went in.

I was actually thinking of times where authors are not explicitly asked to wait, but reviews take a while to get in nonetheless. I'm not blaming anyone for it, but this feels like the typical case to me, so I'm worried that the developer time we have isn't always sufficient to meet our own standards.

I co-authored (and SPEC 0) it as a scientific python community member.

thanks for clarifying this ! I also didn't remember about #2307

There's no issue with us continuing to have Python 3.9 or numpy 1.20; our users should just expect that 3.9 support or Numpy < 1.21 is unstable and YMMV.

Sounds good to me, but do you know if we acknowledge this anywhere ? There's a historic support table in the installation guide but NEP 29/SPEC 0 are not mentioned.

from yt.

matthewturk avatar matthewturk commented on September 21, 2024

I think this is a really important discussion. I will say that I think the most important part is that we try to specify a few lines for where "user-facing" and "infrastructure" changes fall, and that we build into those grace for each other, and the knowledge that we may have different opinions about what constitutes those two categories.

from yt.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.