ROS Resources: Documentation | Support | Discussion Forum | Service Status | Q&A answers.ros.org

Development Practices of Autoware.AI

Autoware
#1

Hi all,
I took a brief hour and reviewed some PRs in https://github.com/CPFL/Autoware/pulls from the last couple of months. I’ve noticed some good things and would like to commend you for:

  1. PRs are getting reviewed. Still too little time spent on the reviews but it seems that at least 1 person looks through them.
  2. CI is being ran and I have not found an instance where it would be abused.
  3. We have 20 functions with tests.

Now the bad:
Someone (e.g. @kfunaoka?) needs to become an ultimate gate keeper. None of the PRs can be merged without his approval. What he should be then strictly looking for:

  1. Is code tested? There is still less that 1 percentile (not even a percent) covered with tests: https://servandogs.gitlab.io/Autoware/.
  2. Is code documented? Better also implement a CI check for it. Autoware.Auto already has it.
  3. Is PR thoroughly reviewed? E.g. if there is 82 files changed in the PR, there can not be only 25 total comments. Also all discussions on the PR must be answered and resolved before the PR can be merged. Don’t just ignore questions.
  4. Developers do not know C++. If you look at this question and answer it is clear that we do not have enough expertise to develop a code that is supposed to be running in a car: https://github.com/CPFL/Autoware/pull/1943#discussion_r252741783.
  5. Someone (e.g. @aohsato @kosuke) must be watching over the architecture For instance a PR like this https://github.com/CPFL/Autoware/pull/1943 where a publisher has been added into many, many nodes can break down the whole system because additional traffic has been added
  6. We keep forking code from ROS core and importing into Autoware: https://github.com/CPFL/Autoware/pull/1791/files. In this case there are parts of rosbag.
  7. This PR has introduced 156 files!!! changed in Autoware: https://github.com/CPFL/Autoware/pull/1950. Could some user possibly with the confidence be updating their develop branch if they saw such a big change?
  8. All new features in release 1.9.0 https://github.com/CPFL/Autoware/releases/tag/1.9.0 have no tests and no documentation.
  9. Features like this https://github.com/CPFL/Autoware/pull/1950 should be clearly marked as experimental and go possibly into a separate folder (called e.g. experimental)
  10. There are currently 238 opened issues and 68 opened PRs. Someone (e.g. @kfunaoka?) should go through and clean them up a bit?
4 Likes
#2

We are working on this. We just discussed it a couple of hours ago as regards our internal processes.

Although originally I was supposed to be that gatekeeper person, I’m trying to put as much of my time into Autoware.Auto as possible. I’m less concerned about code quality in Autoware.AI than I am in Autoware.Auto at the current time.

Until Autoware.AI gets to the point where it’s just a branding and I can manage it without taking up half my time (or more!), we need a process that ensures PRs are good and meet certain basic conditions before they get accepted, without taking up an entire full time developer’s life.

I’m hoping we can set up a “everyone owns the code” situation with a few senior people controlling merging and everyone doing reviews. Ideally I want people developing in their own forks and pulling from those rather than the current mess of branches. Especially experimental things should not get into the mainline until we have confidence from somewhere that they are not going to break everything.

Doing a triage meeting has been on my todo list since the second day I was here.

#3

I have made a summary table of the current state of PRs for new features for v1.11 comment. Some of the PRs have already been merged without suitable unit tests, which makes tracing them extra difficult

#4

We still need to put Autoware.AI in check, it’d be great if someone kept an eye on the code, since the current process hasn’t prevented untested contributions from being merged. From the Autoware.Auto team we need Autoware.AI to have a cleaner roadmap and more controlled development process so that we can plan Autoware.Auto accordingly, especially once it becomes a a dependency of Autoware.AI.

Moroever, many users are still using Autoware.AI and they are not ready to spend time to work on Autoware.Auto, so unless Autoware.AI improves their development process (e.g. more documentation, tests, not changing interfaces in every release, etc.), we risk our current users becoming increasingly frustrated and losing interest in Autoware.Auto in the future.

#5

All good points. I am trying to look over PRs for Autoware.AI but they tend to be huge and beyond my ability to effectively review without understanding the code base properly.

For now, one of the things I’m focusing on is improving the development processes used to that future PRs are more manageable. As part of this I’m going to be proposing new PR guidelines, culling the number of people who have merge permissions, and pruning the repository from a thicket down to a nice shrubbery.

#6

Also it would be great to have an integration testing script, probably manual.

#7

This is the nuclear option, but unfortunately can’t help but think it’s the right one now, there are numerous pull requests being merged in minutes after being submitted without any QA or discussion.

#8

It’s not as nuclear as you may think. There are 113 people with full write access to the repository.

#9

We do have some people working on simulation and one of the goals is a headless simulation CI step. I don’t know what the roadmap is, though, as I haven’t had time to look into that area in detail yet.