search

autowarefoundation / autoware

Autoware - the world's leading open-source software project for autonomous driving
https://www.autoware.org/
Apache License 2.0
9.15k stars 3.05k forks source link

Prefix packages with autoware_ prefix and keep code in autoware namespace #4569

Open esteve opened 7 months ago

esteve commented 7 months ago

Checklist

Description

See https://github.com/orgs/autowarefoundation/discussions/4097

Purpose

Keep Autoware code clearly separate from other projects by adding appropriate namespacing and prefixes

Possible approaches

Add autoware namespace and autoware_ prefix for package names

Definition of done

All packages are prefixed and the code is in the autoware namespace

List of packages in reverse topological order:

xmfcx commented 6 months ago

Following up from:

We should prefix the package folder names with autoware_ too.

The package name in the package.xml file should match its parent folder.

I might have missed something too, open to discussion.

esteve commented 6 months ago

@xmfcx I've been waiting for feedback from the TierIV engineers regarding renaming the folders. @mitsudome-r do you have any updates? Thanks.

xmfcx commented 6 months ago

I would like to change it and see the response in this case. And solve the issues after they arise.

main branch is a development branch and this is expected.

mitsudome-r commented 6 months ago

I think we can move on with changing the package folder name as well. However, we did have some concerns about the changing the package names & include folder structure for the packages in autowarecommon repository and also the packages with tier4 prefix.

https://github.com/autowarefoundation/autoware_common

technolojin commented 4 months ago

@esteve @xmfcx @mitsudome-r

Hello. This is Taekjin Lee, a representative for the package prefix task in the TIER IV perception team. We will start to work from July.

We think the working timing should be aligned to minimize impact between repositories, launchers, and related systems may depends on the package names.

TIER IV perception engineering team will take charge of packages under

technolojin commented 4 months ago

@esteve @xmfcx @mitsudome-r We (TIER IV perception engineering team) decided to separate tasks of the namespace fix and the package name fix. The related PRs will refer this issue, so that the progress can be tracked.

technolojin commented 4 months ago

Detail of https://github.com/autowarefoundation/autoware/issues/4569#issuecomment-2188129685

In /perception, the package name will be changed after all the packages are ready. The separation is to minimize mistakes on namespace changes from package renaming. If those two works are done in the same time, complexity of the work and the test procedure may increases.

When the relevant packages are ready (namespace fix), the package renaming will be done in a concentrated timing.

esteve commented 4 months ago

@technolojin thanks for elaborating, but I have a question about this:

The separation is to minimize mistakes on namespace changes from package renaming. If those two works are done in the same time, complexity of the work and the test procedure may increases.

Why is that? We've done that with the planning packages and many others and we haven't had a problem. What makes the perception packages different? I've moved the header files and renamed the packages for some packages in the perception subsystem in https://github.com/autowarefoundation/autoware.universe/pull/7809 https://github.com/autowarefoundation/autoware.universe/pull/7808 and https://github.com/autowarefoundation/autoware.universe/pull/7804 and it doesn't seem to have caused any issues.

technolojin commented 4 months ago

@esteve There are big differences between sensing/perception and other components.

The sensing and some perception components are absorbing hardware differences. Therefore, the launcher of the systems contains names of packages and those are distributed over multiple repositories. Some packages you mentioned is after virtualization is done, so that those were fine. In TIER IV, we have launchers for each products, launchers for main ECU and embedded devices, launchers and scripts for CI/CD. I believe the situation is similar or same to other autoware users. As TIER IV perception team, we conducts tests on each PRs to ensure the system and CI/CD are working.

This is reason why our team want to lead the work for the package prefix and want to concentrate the name changes. By separating the other required works (namespace, distinguishing shared headers, remove unused dependencies), which can be done per package with minimized changes on launchers, it is possible that the package name change can be done in a short time and shorten overall down time for all relevant system. We need cooperation of code owners to achieve the goal.

I cannot fix the schedule of the package prefix fixes since the namespace work is delayed than my expectation. I will announce here when the schedule is visible.

mitsudome-r commented 4 months ago

@technolojin In order to move things forward, I would like to have the following questions answered:

  1. Could you provide the names of the packages that you are especially concerned about? My understanding is that at least, most launch files are referred by tier4_.*_launch files in autoware.universe repository and abstracted in the autoware_launch repository.

  2. If we create the PR first for all the sensing/perception packages so that TIER IV would only have to review, would that make things easier for you? If it makes easier for you to align the merge timing, then we can wait for merging until all the PRs are approved.

technolojin commented 4 months ago
  1. These are repositories and packages linked each other, over the boundary of the autoware_universe. This list is as far as I know at this moment, but I guess there are more.

@kminoda @YoshiRi @drwnz @yukkysaito @miursh Please let us know if I miss repos or packages.

  1. If there is ongoing PRs that related to those sensing/perception packages, it would me easier to merge with all the related repositories, in a short time.
esteve commented 4 months ago

@technolojin as @TakaHoribe said in https://github.com/orgs/autowarefoundation/discussions/4097#discussioncomment-9234315, the Autoware project shouldn't be limited by one company's concerns, we should be free to work on this task regardless of any external requirement. Autoware shouldn't halt its development because of any internal TierIV repository.

https://github.com/tier4/aip_launcher https://github.com/tier4/edge_auto_launch https://github.com/tier4/edge_auto_jetson_launch TIER IV internal - evaluator TIER IV internal - CI/CD (webauto) TIER IV internal - ML related(?)

These repositories are not part of the Autoware project, so they are not relevant to this discussion.

If there is ongoing PRs that related to those sensing/perception packages, it would me easier to merge with all the related repositories, in a short time.

There are already PRs that address this task (see https://github.com/autowarefoundation/autoware.universe/pull/7809 https://github.com/autowarefoundation/autoware.universe/pull/7808 https://github.com/autowarefoundation/autoware.universe/pull/7804), they are not being reviewed or merged because they impact TierIV's internal development, which shouldn't liimit what the Autoware project can do or not. This is what happens when you track the main branch in a project, breaking changes happen all the time. You can always target a tag to make sure the code does not change.

mitsudome-r commented 4 months ago

@technolojin As a long term solution, I prefer to move some of the evaluator for perceptions to AWF projects so that we have better transparency.

For the short term solution, we can:

  1. Create PRs to add autoware_ prefix for the listed packages
  2. Do reviews for all the PRs and wait for merging until we get approval for all the packages
  3. Once all PRs are approved, you can do any tests that you want to make internally and report the results
  4. Merge the PRs

The above is for AWF repositories. For the TIER IV repositories(like edge_auto_launch), it should be TIER IV's responsibility to when you sync with the upstream AWF repositories.

mitsudome-r commented 4 months ago

@esteve I talked with @technolojin today. He said TIER IV will be responsible for changing the package names and namespace for packages in perception/sensing folder. Could you focus on the other packages for now?

He will post more detailed timeline about expected day to finish all the changes later.

mitsudome-r commented 4 months ago

Also, another thing that @technolojin proposed is to split the modification into two PRs for packages without a library:

This is because changing the namespace for non-library packages wouldn't be a breaking change because it would be pretty much self-contained in the package, whereas changing the package name would be a breaking change for all the launch files that refers to the package.

He said that he would like to make sure that all the non-breaking change is merged first, and then do some regression tests for the PRs with breaking changes all together in short period of time.

One of the drawbacks of splitting the package would be that it makes us harder to track the progress properly, but I think it can be solved by creating a sub-issue to just track sensing/perception with two checkboxes for each package. Something like the following (or it could be a spreadsheet):

I personally don't mind if that makes them easier to review the change and do the tests, but do you have any comments @esteve?

technolojin commented 4 months ago

@esteve @mitsudome-r

I would like to share perception team schedule for those works.

We keep work on namespace changes that are not breaking change. Also, the package name changes can be done within autoware_universe will be done from now (example: https://github.com/autowarefoundation/autoware.universe/pull/7892).

Current estimation for the breaking changes is as the following period. 22-25th of July

esteve commented 4 months ago

@technolojin timeline sounds alright. I'd prefer to make the changes both namespace and package name in the same PR, but I think it's a fair compromise if this helps getting the changes merged as soon as possible. Would it be possible to make the changes per package in each PR? It's easier to track what's being in the process and what's finished.

However, I wouldn't wait until the shared library that you mentioned earlier is complete, but just make the changes as soon as possible and then you can do any extra refactorings after the rename.

technolojin commented 4 months ago

@esteve Thank you for your understanding.

  1. Separating PRs for namespace and package name I see some packages that it is better to change the namespace and package name in the same time because both changes are breaking changes. I would like to respect decision of developers who will work on these. Also, some packages may worked in the same time to avoid complex dependency and conflict. If the work is on multiple packages, it would be better to indicate which packages are the target.

  2. Shared library, directory structure, location of header files

    https://github.com/autowarefoundation/autoware.universe/pull/7804#issuecomment-2205559422 @technolojin I've submitted this PR as a follow-up to https://github.com/autowarefoundation/autoware.universe/pull/7642 to update the package name and move the headers, let me know if the changes are ok. Thanks.

"move the headers" is out of scope of this issue. I understand to refactoring packages in this chance may helpful for overall SW quality improvement. However, it is not mandatory and I want to discuss about the directory structure. Until we discuss about this topic, developers may not relocate the headers as mandatory.

  1. For object tracking packages I mentioned as following.

    https://github.com/autowarefoundation/autoware.universe/pull/7808#pullrequestreview-2157746309 moving headers I am considering the association to be shared in the future. this association part is the same as multi_object_tracker, radar_object_tracker, and tracking_object_merger. Let me do the re-organization later.

I checked differences of the DataAssociation classes on each packages, but there are many differences in variables and configurations. I think it is better to do after the work.

esteve commented 4 months ago

"move the headers" is out of scope of this issue. I understand to refactoring packages in this chance may helpful for overall SW quality improvement. However, it is not mandatory and I want to discuss about the directory structure. Until we discuss about this topic, developers may not relocate the headers as mandatory.

Headers need to be moved anyway, either to private location or to include/autoware/object_merger as part of the refactor task. If you want to keep the headers public, I can move them to include/autoware/object_merger, no problem, but they have to be moved no matter what (we've done this with plenty of other packages successfully, either public or private), so I took the opportunity to move them private because there are no downstream packages that depend on object_merger

technolojin commented 4 months ago

Headers need to be moved anyway, either to private location or to include/autoware/object_merger as part of the refactor task. If you want to keep the headers public, I can move them to include/autoware/object_merger, no problem, but they have to be moved no matter what (we've done this with plenty of other packages successfully, either public or private), so I took the opportunity to move them private because there are no downstream packages that depend on object_merger

Yes, we have to move header include/autoware/object_merger or private. For code owners of perception packages, readability is important. If you can get agreement about proper directory structure for private headers, It can be done in this work. But if it is not, then it need to be done separately.

esteve commented 4 months ago

@technolojin

Yes, we have to move header include/autoware/object_merger or private.

Moving the headers is in the scope of this task, directory structure is part of the rules we're implementing for all packages.

For code owners of perception packages, readability is important.

Readibility is important for all packages, not just perception, so that's why we want all subsystems to follow the same rules, the perception subsystem is not special. Code owners can't override the rules we have established for all the Autoware project, the other subystems are following these rules, the perception subsystem is not different.

If you can get agreement about proper directory structure for private headers, It can be done in this work. But if it is not, then it need to be done separately.

I don't understand this sentence, can you elaborate? What do you mean by getting agreement? The coding guidelines are clear about the directory structure of headers, and moving headers is part of this task, it's not out of scope. The headers structure must match either the public structure (include/autoware/object_merger) or private (src or src/include), see https://autowarefoundation.github.io/autoware-documentation/main/contributing/coding-guidelines/ros-nodes/directory-structure/#include-and-src

technolojin commented 4 months ago

@esteve

If my sentences were not clear to you, I am sorry for that.

The headers structure must match either the public structure (include/autoware/object_merger) or private (src or src/include)

Yes, I agree in this. I am not trying to overwrite those rules.

There are some packages already worked, and the headers are in (include/autoware/package_name) even those are not used outside of the package. https://github.com/autowarefoundation/autoware.universe/tree/36a1d14a90d8f68900a7bc84295c958971fb9c05/planning/autoware_obstacle_cruise_planner/include/autoware/obstacle_cruise_planner https://github.com/autowarefoundation/autoware.universe/tree/36a1d14a90d8f68900a7bc84295c958971fb9c05/planning/autoware_planning_validator https://github.com/autowarefoundation/autoware.universe/tree/36a1d14a90d8f68900a7bc84295c958971fb9c05/localization/autoware_landmark_based_localizer/autoware_landmark_manager/include/autoware/landmark_manager

As like the other components, let the developers and the code owners decide whether they put the headers in the public structure or private. That is what the 'agreement' I want to say.

esteve commented 4 months ago

@technolojin

Yes, I agree in this. I am not trying to overwrite those rules.

Glad we reached an agreement :slightly_smiling_face:

esteve commented 4 months ago

As like the other components, let the developers and the code owners decide whether they put the headers in the public structure or private. That is what the 'agreement' I want to say.

Yes and no. While code owners have the responsibility to review changes, that doesn't necessarily mean that they have the last word when it comes to following the guidelines of the Autoware project, and if only changes from within the perception team are accepted, you risk into turning into an echo chamber. From https://autowarefoundation.github.io/autoware-documentation/main/contributing/coding-guidelines/ros-nodes/directory-structure/#include-and-src

  • Unless you specifically need to export headers, you shouldn't have a include directory under the package directory.
  • For most cases, follow Not exporting headers.
  • Library packages that export headers may follow Exporting headers.

I haven't seen any argument why the headers in the perception subsystem should be public, no downstream packages depend on these headers.

I'm moving headers back to public directory structure for the perception subsystem PRs I submitted, but I still don't understand the reluctance to not follow the Autoware guidelines and I'd appreciate an explanation to understand better your point of view. Thanks.

technolojin commented 4 months ago

@esteve I understand necessity to move non-exporting headers ouside of include folder. I have done that work for relatively simple packages.

and more.

In some complex packages, code owners need time to think its folder structure. The directory structure in src is up to developers, and need agreement of the code owners, who considers its readability. Therefore, I am suggesting separate the moving non-experting headers outside of include work from the current package renaming work. I think it is up to developers.

technolojin commented 4 months ago

@esteve

I'm moving headers back to public directory structure for the perception subsystem PRs I submitted

Thank you for your understanding. I will fix the header locations when overall rename work is done. I reviewed your PRs. Please see my comments.

esteve commented 4 months ago

@technolojin

Thank you for your understanding. I will fix the header locations when overall rename work is done.

Can you elaborate why you need to do it in separate PRs? I'd like to understand your motivation for doing that. Code owners of other subsystems equally as complex as perception have moved headers and renamed packages at the same time.

I reviewed your PRs. Please see my comments.

I've addressed your feedback, can you review my PRs again? Thanks.

technolojin commented 4 months ago

@esteve

can you review my PRs again?

There were missing part in the launchers. I fixed those. https://github.com/autowarefoundation/autoware.universe/pull/7809 https://github.com/autowarefoundation/autoware.universe/pull/7804

why you need to do it in separate PRs

  1. https://autowarefoundation.github.io/autoware-documentation/main/contributing/pull-request-guidelines/#keep-a-pull-request-small-advisory-non-automated This is one reason, and I got the given works can be separated.
  2. I wanted to reduce chance of conflict, or reduce impact of conflict if occurred.
  3. As I mentioned before, the package names are used in other repos. But this is TIER IV business.
esteve commented 4 months ago

@technolojin

There were missing part in the launchers. I fixed those. https://github.com/autowarefoundation/autoware.universe/pull/7809 https://github.com/autowarefoundation/autoware.universe/pull/7804

Thanks!

a-maumau commented 3 months ago

Please be informed that I will start adding the autoware_ prefix to some localization/mapping packages.

youtalk commented 2 months ago

@esteve Is it possible to prioritize renaming the packages under autoware.universe/common? In relation to https://github.com/autowarefoundation/autoware/issues/5077, we would like to complete the common packages first.

esteve commented 2 months ago

@youtalk yes, makes total sense to prioritize autoware.universe/common, I'll refocus on the remaining common packages.

youtalk commented 2 months ago

@esteve Thank you so much!