[yt-dev] Analysis module review process
John ZuHone
jzuhone at gmail.com
Wed Dec 30 14:39:07 PST 2015
It seems to me that something probably ought to be done, because most of the analysis modules are so domain-specific that they often require expertise in that particular area in order to give a proper review beyond just code errors and/or style. This is often what happens, in PR hangouts I often hear “well, the code itself looks good, even though I have no idea what it does."
photon_simulator is an example of this (albeit an extreme one). I’ve contemplated spinning it off into a separate package, given that its size and complexity are both getting rather large, but given that we have more than a handful of users for it I feel that this would be too disruptive.
As an example of how to do “affiliated packages”, AstroPy has such a notion, but I always found the process by which a package achieves this status rather complicated.
http://www.astropy.org/affiliated/ <http://www.astropy.org/affiliated/>
> On Dec 30, 2015, at 5:25 PM, Matthew Turk <matthewturk at gmail.com> wrote:
>
> Hi all,
>
> This is an interesting discussion ... and to be honest, I'm kind of
> left wondering if maybe we should be encouraging more analysis modules
> to be external packages. I came into this thinking that we would want
> to make analysis modules behave more like external packages that get
> bundled, but I'm not sure that's the case after all. Unfortunately,
> we don't *have* a method for defining "affiliated" packages, and we
> certainly don't have (or *want*) an update mechanism for external
> packages that build on yt. But this becomes something of a
> self-reinforcing loop ... Maybe we should come up with something like
> that, so that projects that build on yt can be "advertised" somehow in
> the base package, or something. I am not sure.
>
> Anyway, the responses to this thread make me think perhaps we
> shouldn't lower the review barrier after all -- going from 3 to 2 is
> perhaps just window dressing, and maybe we should encourage the
> analysis modules to have higher bus factors.
>
> -Matt
>
> On Tue, Dec 29, 2015 at 5:09 PM, Nathan Goldbaum <nathan12343 at gmail.com> wrote:
>> I agree. If we can identify maintainers for analysis modules, I think only
>> one additional approval besides the maintainer is fine. If the maintainer
>> proposes the PR, they should try to get at least one person to look over it
>> in detail before it gets merged.
>>
>> On Tuesday, December 29, 2015, Britton Smith <brittonsmith at gmail.com> wrote:
>>>
>>> I agree that the number of approvals is less important than ensuring that
>>> PRs come with appropriate tests and docs. If the PR is a change to an
>>> existing analysis module, we should just make sure the relevant devs are
>>> identified and that they sign off on it. If it's something brand new, I
>>> think 2 or 3 people is probably still a good idea.
>>>
>>> On Tue, Dec 29, 2015 at 4:44 PM, Matthew Turk <matthewturk at gmail.com>
>>> wrote:
>>>>
>>>> Ideally, with the barrier to acceptance reduced, the reviews that come
>>>> in may be deeper and more thoughtful ... and less along the lines of
>>>> "Can somebody just hit the approve button?"
>>>>
>>>> On Tue, Dec 29, 2015 at 4:41 PM, John ZuHone <jzuhone at gmail.com> wrote:
>>>>> +1 for lowering the barrier. At a minimum, tests should pass, and new
>>>>> tests
>>>>> should be encouraged if possible. Docs as well.
>>>>>
>>>>> Not too worried about 1 or 2 approvals. We could try 2 at first to see
>>>>> if
>>>>> this helps things out.
>>>>>
>>>>> On Dec 29, 2015, at 5:39 PM, Cameron Hummels <chummels at gmail.com>
>>>>> wrote:
>>>>>
>>>>> As long as the code being changed is local to an analysis module and
>>>>> not
>>>>> used by other parts of the main yt codebase, yes, I'm all for dropping
>>>>> the 3
>>>>> reviewer requirement to 2. 1 might be pushing it though.
>>>>>
>>>>> On Tue, Dec 29, 2015 at 2:38 PM, Matthew Turk <matthewturk at gmail.com>
>>>>> wrote:
>>>>>>
>>>>>> That's precisely what I had in mind.
>>>>>>
>>>>>> On Tue, Dec 29, 2015 at 4:36 PM, Nathan Goldbaum
>>>>>> <nathan12343 at gmail.com>
>>>>>> wrote:
>>>>>>> On Tue, Dec 29, 2015 at 4:29 PM, Matthew Turk
>>>>>>> <matthewturk at gmail.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>> [+-][01] on reducing review overhead for analysis modules?
>>>>>>>
>>>>>>>
>>>>>>> Does this just mean reducing the number of PR reviewers before we
>>>>>>> merge
>>>>>>> pull
>>>>>>> requests? I'd be ok with that, but just want to clarify what you
>>>>>>> have in
>>>>>>> mind.
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> yt-dev mailing list
>>>>>>> yt-dev at lists.spacepope.org
>>>>>>> http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
>>>>>>>
>>>>>> _______________________________________________
>>>>>> yt-dev mailing list
>>>>>> yt-dev at lists.spacepope.org
>>>>>> http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Cameron Hummels
>>>>> NSF Postdoctoral Fellow
>>>>> Department of Astronomy
>>>>> California Institute of Technology
>>>>> http://chummels.org
>>>>> _______________________________________________
>>>>> yt-dev mailing list
>>>>> yt-dev at lists.spacepope.org
>>>>> http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> yt-dev mailing list
>>>>> yt-dev at lists.spacepope.org
>>>>> http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
>>>>>
>>>> _______________________________________________
>>>> yt-dev mailing list
>>>> yt-dev at lists.spacepope.org
>>>> http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
>>>
>>>
>>
>> _______________________________________________
>> yt-dev mailing list
>> yt-dev at lists.spacepope.org
>> http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
>>
> _______________________________________________
> yt-dev mailing list
> yt-dev at lists.spacepope.org
> http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.spacepope.org/pipermail/yt-dev-spacepope.org/attachments/20151230/c451fbd0/attachment.htm>
-------------- next part --------------
_______________________________________________
yt-dev mailing list
yt-dev at lists.spacepope.org
http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
More information about the yt-dev
mailing list