Move CUDA-dependent packages to a separate repository

esteve commented 1 year ago

Checklist

[X] I've read the contribution guidelines.
[X] I've searched other issues and no duplicate issues were found.
[X] I've agreed with the maintainers that I can plan this task.

Description

As discussed in https://github.com/autowarefoundation/autoware/issues/3222#issuecomment-1478085589, in order to be able to build Autoware.Universe with the available packages in the ROS index, we decided to move those packages that depend on CUDA to a separate repository

Purpose

Be able to compile a subset of the Autoware.universe without depending on CUDA

Possible approaches

Move the the packages that depend on CUDA to a separate repository

Definition of done

There are no packages that depend on CUDA in Autoware.universe

miursh commented 1 year ago

Is there any options not to separate CUDA dependent packages? Because of reasons below, I would not want to separate just a part of perception packages

If some of perception packages are placed in the different repository, it is very confusing for developers.
- For example, when we want to add some change, we might need to make 3-4 PRs to different repository, such as new perception repo, autoware.unvierse, autoware_launch, and perception utils (new repo too?)
Very difficult to understand especially for new users
There might be some dependency between autoware.universe and separated packages. If there is many dependency, github CI might not work well.

xmfcx commented 1 year ago

Name of the new cuda packages repository

What are your naming choices about this new repository?

I'd go for cuda_autoware_pkgs, what are your suggestions?

Make naming consistent

Also after moving the packages, we should make the naming conventions uniform too.

And for that my proposal is:

cuda_trt_package_name
cuda_package_name (we don't have any cuda only packages yet, just convention)
cuda_tvm_package_name (this is also preserved for future, we will have a wide discussion on what to do with tvm packages in general soon)

Handling of TVM packages

Also we should move tvm packages to tvm_autoware_pkgs and they will contain tvm packages for non-cuda targets.

This topic will also be discussed by @ambroise-arm @angry-crab @mitsudome-r @esteve @kenji-miyake and me.

to separate or not to separate

Edit: @miursh @yukkysaito I've just saw your comment after refreshing. Yes, I will think about not separating too. I understand the concerns, this is an issue with all separated repositories.

kenji-miyake commented 1 year ago

I'm sorry for joining late.

To give you some additional information, splitting repositories helps us reduce CI time. There is no need to test all packages with CUDA. https://github.com/autowarefoundation/autoware.universe/blob/541122f6d214db36b1358c6ba02be2f8d8f7b089/.github/workflows/build-and-test-differential.yaml#L15-L17

But I understand splitting repositories may increase maintenance costs and troubles.

I think it's okay not to split repositories right now, we should do the following at least:

As @xmfcx says, make package names consistent, which helps us ignore packages in CI.
- Related: https://github.com/jspricke/ros-deb-builder-action/issues/19
Plan a migration timeline to be CUDA-independent for hardware-agnostic, which I believe is one of the goals of Autoware.

I'll discuss how to proceed with this also in TIER IV.

esteve commented 1 year ago

@kenji-miyake another reason for splitting the CUDA-related packages to another repository is because when/if we submit Autoware to the ROS buildfarm that Open Robotics maintains (https://build.ros.org/), all the packages in the repository are submitted and they need to be distributable and not covered by a proprietary license.

However, following @mitsudome-r 's rationale of only submitting Autoware.core and not all of Autoware.universe to the ROS buildfarm, we can either leave CUDA packages in universe, or move them in a separate repository, but we can't migrate them to Autoware.core.

kenji-miyake commented 1 year ago

However, following @mitsudome-r 's rationale of only submitting Autoware.core and not all of Autoware.universe to the ROS buildfarm, we can either leave CUDA packages in universe, or move them in a separate repository, but we can't migrate them to Autoware.core.

Yes, I think so, too.

kenji-miyake commented 1 year ago

As a result of TIER IV's internal discussion, we'd like to propose the following actions:

To resolve the build issues in Debian package release, we'll rename existing packages, and don't move packages to another repository yet.
- Having a unified naming convention can help us ignore specific packages in CI.
- Moving packages to another repository has significant impacts, so we need step-by-step improvements.

For that,

Define naming conventions.
- For CUDA/TensorRT packages, use _trt.
- For TVM packages, use _tvm.
Rename existing packages to follow the defined conventions.
- In addition, maybe old-style packages should be fixed.
- For example, https://github.com/autowarefoundation/autoware.universe/tree/aba954f30124155afe15ba4f216076101a125265/perception/lidar_apollo_segmentation_tvm_nodes can be merged into https://github.com/autowarefoundation/autoware.universe/tree/aba954f30124155afe15ba4f216076101a125265/perception/lidar_apollo_segmentation_tvm.
Write the naming convention rules in the contribution guidelines.

@xmfcx @esteve cc @mitsudome-r What do you think about this? If it looks good, we'd appreciate it if @esteve -san would work on these tasks as part of the Debian package issue.

Regarding the long-term milestones, I think as follows:

We should continue discussing how we should utilize TVM.
Each one should try to gain a deeper understanding around TVM through learning/experiments.

xmfcx commented 1 year ago

I agree that we can start by renaming.

For CUDA/TensorRT packages, use _trt.

For TVM packages, use _tvm.

I'd prefer prefix over suffix. But I'm OK either way.

Also merging of _nodes double-packages will simplify the maintenance in general.

esteve commented 1 year ago

I agree that we can start by renaming.

For CUDA/TensorRT packages, use _trt.

For TVM packages, use _tvm.

I'd prefer prefix over suffix. But I'm OK either way.

I also agree with using suffixes, makes it clear that they are an extension to the base packages or that they are based on other packages.

Also merging of _nodes double-packages will simplify the maintenance in general.

I'm afraid I don't follow you here, can you elaborate? Thanks.

xmfcx commented 1 year ago

I'm afraid I don't follow you here, can you elaborate? Thanks.

For example,

could be merged into a single package.

esteve commented 1 year ago

@xmfcx thanks for elaborating.

I can't clearly remember what was the rationale for having _nodes packages, I think it was inherited from Autoware.Auto because Apex.AI wanted the core functionality separate from the node code because it was incompatible with Apex.OS, but I might be making this up, I don't remember.

Anyway, I agree with you, it'd simplify our tree, especially now that we're enforcing all nodes to be components, so they all have uniform instantiation.

xmfcx commented 1 year ago

I can't clearly remember what was the rationale for having _nodes packages, I think it was inherited from Autoware.Auto because Apex.AI wanted the core functionality separate from the node code because it was incompatible with Apex.OS, but I might be making this up, I don't remember.

From what I remember, it was to separate ROS libraries from the rest of the packages. And it's a good practice in theory because the library can also be reused by other packages too. But in the implementation, this wasn't adhered strictly. So I think it lost its purpose along the way.

Found the source: https://autowarefoundation.gitlab.io/autoware.auto/AutowareAuto/contributor-guidelines.html#autotoc_md24

This design pattern helps to promote separation of concerns and code re-use. The core and the ROS node(s) can be implemented in separate packages; e.g. foo and foo_nodes. There are some trivial cases where a simple ROS Node that does not require a "core" are acceptable but these should be the exception, not the rule.

Also I used to be 3-tier before (but I think it's just a matter of semantics for the executable): https://gitlab.com/autowarefoundation/autoware.auto/AutowareAuto/-/merge_requests/255#note_316711298

stale[bot] commented 1 year ago

This pull request has been automatically marked as stale because it has not had recent activity.

autowarefoundation / autoware.universe