Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make development process more transparent #238

Open
EwoutH opened this issue May 5, 2019 · 10 comments

Comments

Projects
None yet
3 participants
@EwoutH
Copy link
Contributor

commented May 5, 2019

@hassount In my opinion, this is extremely important to keep the community involved to profit from the benefits of open source. Way too much of the development process is now behind the scenes, which makes entry barriers higher and excludes proper outside contributing, testing and reviewing.

Some guidelines that should be seriously considered:

  • Open a pull request as soon as you can. No matter how small the feature. WIP's are also great, as long as you don't ask for a formal review until it's done. It helps people understand the code and the process, and testing and reviewing multiple small updates far easier most times.
  • Don't bundle multiple features. Have a feature B that depends on A? Open a pull request for A, continue working on B and only if B is done before A is merged open a pull request for both. We saw huge code blocks merged with #185 and #235, which confused the shit out of everybody not directly involved in those features.
  • Don't squash and merge multiple contributors or multiple features. Proper credit should be given and it makes it impossible to test for regressions by bisecting.
  • Keep cycle times short. It's extremely demotivating if PR's are open for weeks without review. Code reviews should have priority over code development.

There is probably much more to say about the process optimization but this should be a decent start. Additions are welcome of course.

This is also a great read: Open Source Development Guidelines

@EwoutH EwoutH changed the title Make development process more transparten Make development process more transparent May 5, 2019

@EwoutH

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

For example, I see a lot of development going on in different branches at https://github.com/hguermaz/SVT-AV1 by a lot of different contributors. Can't this be done in the OpenVisualCloud repository? Or at least PR's could be opened way faster, for individual features.

@hassount

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

@EwoutH
Thank you for your constructive feedback and the additional resources. such is greatly appreciated.

I totally agree that smaller separate PRs would allow for a more transparent development. SVT-AV1 has two main development targets right now, that may or may not change as we go through: Be feature complete, and maintain a computational performance that would allow for real-time deployments of AV1 as soon as possible.

At this point we are working on implementing major features that will impact almost every piece of the code. In the case of #235 for example, the MRP feature has touched almost every single file in the project and has impacted all presets, so we needed to implement the rest of the features within the same context, test and fix issues based on the new structure, align everything to the master (that has changed in the mean-time) and finally re-tune all presets. We found that it is almost "impossible" to achieve such work flow and make the improvements in speed / quality that we are targeting within a very tight timeline (a few weeks) while going through small PRs that might need to be changed / overwritten within a day or two as other features move in. We already spend days merging and integrating features to get a coherent code base. Git would already fail to merge after even 1 feature as there are so many conflicts and so, all of this would have to be done manually (not squashing commits is already not a possibility, thus we keep the authors names on the commit message)

As you have already seen in the repo that you have pointed to, we need to make hundreds of commits / multiple branches to integrate everything together in an orderly fashion. Because we are still under development, there are still features that will be causing some major code changes similar to the previous PR.

The Trello board: https://trello.com/b/g0lD2blK/svt-av1 does contain the features that we are currently working on and scheduled for the next commits and is usually is updated quite frequently.

Moving to the solution aspect of it, I agree that we should try to find a middle ground in between the two extremes to make this work externally as well as internally.

  • Creating a branch within the same repo and using that as a base, might be an option, I will take that suggestion to the maintainers to see what we can do there.
  • Reducing the PR cycle is another front that we are trying to work on, we have already upgraded Jenkins to perform more simultaneous builds to allow the code reviews and checks to be faster. That said, more reviewers are welcome here to make this a faster process. Asking developers to review and write code within the same cadence would probably not work as they are under pressure to produce a better quality faster product at a monthly cadence. While I agree that reviewing PRs quickly is an important aspect, I don't essentially that reviewing PRs has a higher priority than working on the code for the developers.

Please let me if you have further suggestions to improve this process now that you have a little more context to this.

@EwoutH

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

@hassount Thank you for the extensive reply and explanation. The last two days I already saw the feedback being applied! Some more ideas:

  • A development branch within the OpenVisualCloud/SVT-AV1 repository would be a great indeed.
  • Make the Jenkins server public
  • Switch from Trello to GitHub's Project Boards
  • Have discussions on public media as much as possible. In GitHub issues and PR's themself, but a public real-time chat (Slack, Discord, IRC or something else) would also be extremely important. See #93.

@spawlows First of all, thanks for all the amazing assembly code you wrote so far. Could you make a list with modules that could be improved with AVX2 assembly in a separate issue? Than we can track which modules need to be improved, and who is doing it. This makes it also easier for other developers to contribute. It could looks something like this: https://code.videolan.org/videolan/dav1d/issues/78

@jbkempf

This comment has been minimized.

Copy link

commented May 13, 2019

Seeing #242 I doubt the message went through.
This project commit log is a mess and the PR are un-reviewable.

@EwoutH

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

@hassount Now that the first release is tagged (0.5.0), milestones should be created for the next releases. It

  • A 0.5.1 milestone to which all critical bugs are added
  • A 0.6.0 milestone to which all features that have priority and could be integrated in the short term are added

I think a minor release should be tagged every month or so, and point releases as much as needed when critical bugs are fixed.


@spawlows First of all, thanks for all the amazing assembly code you wrote so far. Could you make a list with modules that could be improved with AVX2 assembly in a separate issue? Than we can track which modules need to be improved, and who is doing it. This makes it also easier for other developers to contribute. It could looks something like this: https://code.videolan.org/videolan/dav1d/issues/78

@spawlows What do you think?

@hassount

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

@ all,
Thank you for the feedback, I do agree that there will need to be quite a bit of improvement to the process and we'll work on it with your constructive feedback as we go.

Still discussing internally.

  • Make the Jenkins server public

SVT is only using Travis, we are able to see the results publicly under the checks tab (e.g. https://github.com/OpenVisualCloud/SVT-AV1/pull/253/checks) is there more to this ?

This switch would need to be discussed further as many projects under the OpenVisualCloud would have to follow SVT-AV1 which might not be possible at this point. In the mean time, a suggestion is to create an issue for every feature the team is working on adding an "work in progress" label and reference it in the PR. If we can not move the branches to the main repo, we could maybe point to the branch where the work is being done (if possible, as sometimes the work is done in multiple branches), any thoughts ?

  • Have discussions on public media as much as possible. In GitHub issues and PR's themself, but a public real-time chat (Slack, Discord, IRC or something else) would also be extremely important. See #93.

Still discussing internally.

@spawlows First of all, thanks for all the amazing assembly code you wrote so far. Could you make a list with modules that could be improved with AVX2 assembly in a separate issue? Than we can track which modules need to be improved, and who is doing it. This makes it also easier for other developers to contribute. It could looks something like this: https://code.videolan.org/videolan/dav1d/issues/78

We don't have a complete list so far, we would optimize kernels once they show up on the profiling consuming a lot of cycles. That said, following the approach above, we would create a github issue for the kernels that are work in progress ?

@EwoutH

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

Still discussing internally.

This switch would need to be discussed

Still discussing internally.

I'm going to suggest something extreme here, that probably solves everything, so please take a seat:

  • Why not discuss them publicly?

If there are compelling reasons to keep things the way they're, then the community will agree. If there are large costs and low gains, the community will understand. The SVT encoders need to be a shared project with the community, or the point (and profits) of open source is lost at all. Otherwise please call yourself Google and make this repo just another code dump. :)

@hassount

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

@EwoutH
That's a very good question:
For example: Switch from Trello to GitHub's Project Boards: right now, many teams that are working on the OpenVisualCloud are using Trello and this was a decision taken at a broader Intel level than the SVT teams. If we want to move SVT to another system, we would have to meet again with the teams and discuss what are the pros and cons of doing such. Some pros and cons could not be discussed in public as they contain privileged customer information that is why we can not put this as a simple discussion on github.

@EwoutH

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

@hassount @kylophone Could you make sure you and your team members check in to IRC every once in a while (or better: always when they're at work). It keeps the lines to the community short.

I would like to discuss somethings about milestones/releases, ABI stability, SIMD integration and documentation for example.

@hassount

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

@EwoutH
I have been on for the past couple of days and there weren't any questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.