Hi everyone, Before writing this up in the YTEP, I wanted to get a sense of people's feelings on lowering the barrier to entry for changes to analysis modules. Often these are maintained by a smaller number of people and don't touch much of the rest of the code, so changes can be hard to get pushed through. An alternate thing we could try out would be spinning these out into their own packages, which we could provide helpers or hooks to download and install. A big advantage to having them in the main repo is that then everything comes in the box. [+-][01] on reducing review overhead for analysis modules? _______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
On Tue, Dec 29, 2015 at 4:29 PM, Matthew Turk
[+-][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@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
That's precisely what I had in mind.
On Tue, Dec 29, 2015 at 4:36 PM, Nathan Goldbaum
On Tue, Dec 29, 2015 at 4:29 PM, Matthew Turk
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@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
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
That's precisely what I had in mind.
On Tue, Dec 29, 2015 at 4:29 PM, Matthew Turk
wrote: [+-][01] on reducing review overhead for analysis modules?
Does this just mean reducing the number of PR reviewers before we merge
On Tue, Dec 29, 2015 at 4:36 PM, Nathan Goldbaum
wrote: pull requests? I'd be ok with that, but just want to clarify what you have in mind.
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@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@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
+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
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
mailto:matthewturk@gmail.com> wrote: That's precisely what I had in mind. On Tue, Dec 29, 2015 at 4:36 PM, Nathan Goldbaum
mailto:nathan12343@gmail.com> wrote: On Tue, Dec 29, 2015 at 4:29 PM, Matthew Turk
mailto:matthewturk@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@lists.spacepope.org mailto:yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org mailto:yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-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 http://chummels.org/ _______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
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
+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
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
wrote: That's precisely what I had in mind.
On Tue, Dec 29, 2015 at 4:36 PM, Nathan Goldbaum
wrote: On Tue, Dec 29, 2015 at 4:29 PM, Matthew Turk
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@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@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@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
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
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?"
+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
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
On Tue, Dec 29, 2015 at 4:41 PM, John ZuHone
wrote: the 3 reviewer requirement to 2. 1 might be pushing it though.
On Tue, Dec 29, 2015 at 2:38 PM, Matthew Turk
wrote: That's precisely what I had in mind.
On Tue, Dec 29, 2015 at 4:36 PM, Nathan Goldbaum
wrote:
On Tue, Dec 29, 2015 at 4:29 PM, Matthew Turk
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@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@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@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
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
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
javascript:_e(%7B%7D,'cvml','matthewturk@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?"
+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
javascript:_e(%7B%7D,'cvml','chummels@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
On Tue, Dec 29, 2015 at 4:41 PM, John ZuHone
javascript:_e(%7B%7D,'cvml','jzuhone@gmail.com');> wrote: the 3 reviewer requirement to 2. 1 might be pushing it though.
On Tue, Dec 29, 2015 at 2:38 PM, Matthew Turk
javascript:_e(%7B%7D,'cvml','matthewturk@gmail.com');> wrote: That's precisely what I had in mind.
On Tue, Dec 29, 2015 at 4:36 PM, Nathan Goldbaum <
nathan12343@gmail.com javascript:_e(%7B%7D,'cvml','nathan12343@gmail.com');>
wrote:
On Tue, Dec 29, 2015 at 4:29 PM, Matthew Turk
javascript:_e(%7B%7D,'cvml','matthewturk@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@lists.spacepope.org javascript:_e(%7B%7D,'cvml','yt-dev@lists.spacepope.org'); http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org javascript:_e(%7B%7D,'cvml','yt-dev@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@lists.spacepope.org javascript:_e(%7B%7D,'cvml','yt-dev@lists.spacepope.org'); http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org javascript:_e(%7B%7D,'cvml','yt-dev@lists.spacepope.org'); http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org javascript:_e(%7B%7D,'cvml','yt-dev@lists.spacepope.org'); http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
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
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
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
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
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
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
wrote: That's precisely what I had in mind.
On Tue, Dec 29, 2015 at 4:36 PM, Nathan Goldbaum
wrote: On Tue, Dec 29, 2015 at 4:29 PM, Matthew Turk
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@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@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@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
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
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
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
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
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
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
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
wrote: That's precisely what I had in mind.
On Tue, Dec 29, 2015 at 4:36 PM, Nathan Goldbaum
wrote: > On Tue, Dec 29, 2015 at 4:29 PM, Matthew Turk > > 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@lists.spacepope.org > http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org > _______________________________________________ yt-dev mailing list yt-dev@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@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
The benefits of an "affiliated package" system (although I agree, that
process is a bit complicated) is that things can be versioned
externally, externally published, etc, and we can slim down some
domain-specificity in the core package. The big downside I think
would be reducing discoverability, and not bundling would make it
harder to encourage folks to use the other packages. That can be
mitigated the way astropy does, with things coming into their repo,
being listed on their projects page, etc. What do people think about
that?
On Wed, Dec 30, 2015 at 4:39 PM, John ZuHone
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/
On Dec 30, 2015, at 5:25 PM, Matthew Turk
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
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
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
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
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
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
wrote: That's precisely what I had in mind.
On Tue, Dec 29, 2015 at 4:36 PM, Nathan Goldbaum
wrote: On Tue, Dec 29, 2015 at 4:29 PM, Matthew Turk
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@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@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@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
I guess it's not clear to me what the difference is between a package that
depends on yt and an "affiliated" package.
The nice part about analysis modules being in yt proper is that it helps
prevent breakage in the module due to changes in yt (although ideally
breaking changes in yt aren't supposed to happen) and hopefully also
improves testing in the analysis modules, since they can rely on our
testing infrastructure.
That said, if dealing with the yt patch process is too onerous, maybe it
would be a good idea to have better support on our website or the docs
regarding externally maintained packages that depend on yt.
On Wednesday, December 30, 2015, Matthew Turk
The benefits of an "affiliated package" system (although I agree, that process is a bit complicated) is that things can be versioned externally, externally published, etc, and we can slim down some domain-specificity in the core package. The big downside I think would be reducing discoverability, and not bundling would make it harder to encourage folks to use the other packages. That can be mitigated the way astropy does, with things coming into their repo, being listed on their projects page, etc. What do people think about that?
It seems to me that something probably ought to be done, because most of
On Wed, Dec 30, 2015 at 4:39 PM, John ZuHone
javascript:;> wrote: 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/
On Dec 30, 2015, at 5:25 PM, Matthew Turk
javascript:;> 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
javascript:;> 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
javascript:;> 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
javascript:;> 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
javascript:;> 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
javascript:;> 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
javascript:;> wrote: That's precisely what I had in mind.
On Tue, Dec 29, 2015 at 4:36 PM, Nathan Goldbaum
javascript:;> wrote: On Tue, Dec 29, 2015 at 4:29 PM, Matthew Turk
javascript:;> 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@lists.spacepope.org javascript:; http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org javascript:; 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@lists.spacepope.org javascript:; http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org javascript:; http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org javascript:; http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org javascript:; http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org javascript:; http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org javascript:; http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org javascript:; http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
Sorry to jump in on this thread relatively late. I generally like the idea
of the 'affiliated' analysis modules. One reason to support such a system
is that it would promote the idea of people writing papers to describe just
that specific module that are separate from the yt code papers. I think
that's a good thing - it helps people to get credit for their analysis tool
development work, particularly in the eyes of our stodgier colleagues who
may be looking at CVs for various employment reasons, and it also helps to
justify continued support of yt since there are these useful (and citable)
tools that use yt as their infrastructure. It might also help to encourage
people to develop and contribute their analysis tools back to the
community.
That said, I really do worry about the testing/maintenance aspect. yt has
very nice testing infrastructure, and in terms of maintenance it's fairly
common for people to move on to very different projects or leave the field
altogether, and if many analysis tools are entirely separate then they may
experience bit rot or disappear entirely. Maybe the solution is to keep
analysis tools in the yt repository so that they can lean on yt's testing
framework, but (A) use a consistent import scheme that makes it clear that
you are using something that's outside of the core yt infrastructure (I
think that the photon_simulator code is a good example of something like
this -- 'from yt.analysis_modules.photon_simulator.api import *'), and (B)
add some sort of marking in the documentation that indicates this is a
module that is somewhat separate and maintained by an individual or a
subgroup?
On Wed, Dec 30, 2015 at 5:41 PM, Matthew Turk
The benefits of an "affiliated package" system (although I agree, that process is a bit complicated) is that things can be versioned externally, externally published, etc, and we can slim down some domain-specificity in the core package. The big downside I think would be reducing discoverability, and not bundling would make it harder to encourage folks to use the other packages. That can be mitigated the way astropy does, with things coming into their repo, being listed on their projects page, etc. What do people think about that?
It seems to me that something probably ought to be done, because most of
On Wed, Dec 30, 2015 at 4:39 PM, John ZuHone
wrote: 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/
On Dec 30, 2015, at 5:25 PM, Matthew Turk
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
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
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
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
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
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
wrote: That's precisely what I had in mind.
On Tue, Dec 29, 2015 at 4:36 PM, Nathan Goldbaum
wrote: On Tue, Dec 29, 2015 at 4:29 PM, Matthew Turk
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@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@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@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
participants (6)
-
Brian O'Shea
-
Britton Smith
-
Cameron Hummels
-
John ZuHone
-
Matthew Turk
-
Nathan Goldbaum