search

sunpy / sunkit-instruments

A SunPy-affiliated package for solar instrument-specific tools.
https://docs.sunpy.org/projects/sunkit-instruments/
BSD 3-Clause "New" or "Revised" License
11 stars 14 forks source link

Instrument branches #7

Open DanRyanIrish opened 4 years ago

DanRyanIrish commented 4 years ago

As this package is new, some instrument subpackages may need rapid development and slightly looser standards on individual PRs. Therefore, in order that multiple haphazard instrument-specific development efforts do not turn the package into an unmanageable mess, development should be modularized between instruments.

Proposed Procedure: there should be instrument branches on which people can collaborate and contribute code for a specific instrument, e.g. iris. The contribution procedure would then be the following for different scenarios:

Implementation of a new feature or roadmap for a certain instrument

Feature required for whole package, e.g. retemplating

Bugfix on instrument branch not yet in master

Bugfix on master for specific instrument

Need for change outside of instrument sub-packages discovered as part of instrument specific development.

DanRyanIrish commented 4 years ago

@hayesla any thoughts?

wtbarnes commented 4 years ago

I can see the logic behind this, but doesn't this just offload the problem to the branches rather than the individual PRs? By creating separate branches, you're allowing the individual instrument efforts to potentially diverge quite a bit and the longer you hold off on merging them into master (or rebasing them), the worse that situation could become. I see your point about being able to iterate on a branch before putting it into master, but why not just do this iteration on the PR? Why merge it at all if the tests aren't passing? Alternatively, we could be a bit more lax about what gets merged and when.

I think this package should be moving (and releasing) fast enough that we shouldn't need to add this additional complication (this also places a larger burden on the maintainers and the instrument teams who may not be as familiar with a git-based workflow). This is part of the point of moving it out of core.

I'm not dead set against this idea, but I think we should be cautious about introducing an extra complication into our dev workflow. It is complicated enough already! 😅

DanRyanIrish commented 4 years ago

I can see the logic behind this, but doesn't this just offload the problem to the branches rather than the individual PRs?

I think this does add extra benefit. With this package there are possibly going to be many parallel development efforts by potentially quite different dev teams. They each may work at their own pace. In the early stages of development, they may want to merge PRs onto a common branch that aren't complete works into order to collaborate in getting the subpackage up to a working standard faster or more easily. Otherwise, developers may be having to pull random assortments of each other's branches to stay up to date. The instrument branch centralises that location and cuts down on confusion and divergence. The instrument branch also makes sure that only when the work is in a releasable state (or quite close to it) does the code make it into master. This means master is more likely to always be in (or close to) a release branch. This can help prevent the release of team's work being delayed because other teams have not got all their code in master to a releasable state.

By creating separate branches, you're allowing the individual instrument efforts to potentially diverge quite a bit and the longer you hold off on merging them into master (or rebasing them), the worse that situation could become.

Not if you regularly pull both master and the instrument branch. This is already required in how we work except that there is only one branch to update from, i.e. master. Adding a 2nd isn't a big overhead and reduces divergence, especially since most of the development between subpackages should be independent.

I see your point about being able to iterate on a branch before putting it into master, but why not just do this iteration on the PR?

As said above, multiple people want to help develop the subpackage to its first releasable state. This will likely be the case for what I have in mind for the IRIS subpackage. The instrument branch creates a sandbox in which subpackage teams can collaborate safely and quickly without polluting master for other subpackage teams. If this was done on a single PR, it would lead to multiple PRs to that PR and would get more messy and harder to keep track of.

Why merge it at all if the tests aren't passing? Alternatively, we could be a bit more lax about what gets merged and when.

In early stages of development there may not always be comprehensive tests to catch all bugs. In my experience the reality is that testing often does not catch up with the code until the subpackage is a little more mature. This is often because there is evolution of the package structure, API, and scope in the early stages and in reality people are inhibited from developing rigorous tests until there has been convergence on this.

I think this package should be moving (and releasing) fast enough that we shouldn't need to add this additional complication (this also places a larger burden on the maintainers and the instrument teams who may not be as familiar with a git-based workflow). This is part of the point of moving it out of core.

Unless the development efforts are modularized, this may not be true because without the instrument branches to act as staging areas, one instrument subpackage could hold up the whole package because they have introduced bugs or left features half finished on master. This would actually slow down the iteration of the overall package.

I'm not dead set against this idea, but I think we should be cautious about introducing an extra complication into our dev workflow. It is complicated enough already! 😅

This model is not that dissimilar to how some in industry work. A dev team has a common branch that they work on. When they are ready for release, it gets pushed up to the testing branch where a different team performs rigorous testing. When it passes their process it gets pushed to a release branch. Depending on how you want to think of it, in this model, master becomes the testing/release branch, while the instrument branches are the dev team's branch.

I see the benefits of this system mainly in the early stages of a subpackage's development where, as with sunpy core, it may be a little more like the "wild west" when it comes to merging PRs. Once v0.1 of sunkit-instruments has been released, we could re-assess whether this system still adds benefits.