build(tvm_utility): remove download logic from CMake and update documentation

[lexavtanke]

Description

See #3137

According to discussion above remove downloading logic from building stage. For now it will be possible to download model files either by ansible or manually as explained in this PR

Also documentation about the package is updated to remove mentions about downloading. Kindly ask for maintainers to read throw the doc to check that it still consistent especially Security consideration part.

Tests performed

Not applicable.

Effects on system behavior

Not applicable.

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

• I've confirmed the contribution guidelines.
• The PR follows the pull request guidelines.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

• The PR follows the pull request guidelines.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

• There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

[esteve]

If the artifacts are required for compilation, then they should be part of the source tree (via git-lfs for example if the files are too big), otherwise it won't be possible to build Debian packages.

[lexavtanke]

Actually It is a bit confusing for now. Because this packages don't seem to build anyway now because DOWNLOAD_ARTIFACTS flag is disabled by default and this PR kinda show it because last changes was made several months ago and bug was hidden. So here I just follow the same logic.

And as I understand before we are going to use ansible to provide artifacts. And for packages with tenosorrt support It is more or less straight forward process as all model conversion from onnx to TRT happens later on the first run not on the build.

For TVM it is a bit more complicated as it provide models in already compiled form and to use it in the package you will need to provide some model files to build it.

But if I understand you correctly here. You propose that there should be some default artifacts for all TVM packages which should be part of the source tree and the only way to do it is git-lfs because for now deploy_param.params file weights 10-20 Mb usually. And If user will want to provide his own models wouldn't it be messy a bit ?

I think there is a kinda similar way to complile the model to TVM before run by the user same as with TRT but I'm not so familiar with TVM.

And what is the idea behind the Debian packages? Should user be able to use them of the shelf with the build in model? I guess in many cases user with have to use model trained on his own data.

[esteve]

For TVM it is a bit more complicated as it provide models in already compiled form and to use it in the package you will need to provide some model files to build it.

As far as I know, we only need the headers for building the packages, not the full models.

And what is the idea behind the Debian packages? Should user be able to use them of the shelf with the build in model? I guess in many cases user with have to use model trained on his own data.

The Debian packages are a way for users to install Autoware in production environments without having to build it themselves, much like how ROS is distributed. Eventually, Autoware can become part of the ROS distribution, but that will come at a later stage.

[ambroise-arm]

I don't understand why it bothers anyone that there is an option to download default-compiled models. As you see the default is to not download the models, and there is already an option for the user to provide their own compiled/tuned models; so to me it looks like it already adheres to the proposed plan in #3137.

[lexavtanke]

ambroise-arm The main issue with current approach that it will produce empty Debian package by default as there is no ansible step in their build. And it is not possible to download or provide models in other way. They should be part of the source tree.

May be you can propose the other way to deal with it? May be it is possible to move model compilation step further as it done with onnx2trt conversion? So the model files doesn't need for package compilation.

For now it was offered in Software WG to add header files for default model to the source tree. And the model weights will be provided by the user or downloaded by ansible script. If user want to use his models he will need to rebuild the package or just provide the deploy_param.params file if he just retrain the model.

Correct me, please, if get wrong how TVM works in general.

[esteve]

@ambroise-arm

I don't understand why it bothers anyone that there is an option to download default-compiled models. As you see the default is to not download the models, and there is already an option for the user to provide their own compiled/tuned models; so to me it looks like it already adheres to the proposed plan in #3137.

Requiring packages to have access to internet during build step can make them vulnerable to supply chain attacks, in addition to not being able to make builds reproducible (e.g. for audits). For example, the Debian and ROS buildfarms restrict access to the internet for packages so they must be built in isolation. The end goal is so that all that logic for downloading artifacts done in CMake is removed and instead use ansible (or instruct the user to download the artifacts manually).

[ambroise-arm]

The main issue with current approach that it will produce empty Debian package by default as there is no ansible step in their build.

I don't understand how removing the option to download models with cmake solves this. The problem is that we lack an ansible step, not that there is a cmake download step as an option. At least from what I see. Is the ansible step mutually exclusive with the cmake download step? I don't know what this ansible step would look like (if you have example code somewhere I am interested), but I suppose it can be used together with the other ways to get the artifact, in the same way that the cmake logic currently prefers user-provided models, and if there are none try to download one.

May be it is possible to move model compilation step further as it done with onnx2trt conversion? So the model files doesn't need for package compilation.

Yes, but not with the current implementation. I had started to rework the way tvm_utility worked to allow to remove the need for the artifact to be present at compile time, but I didn't see much interest in TVM packages so I parked the work. Maybe I could continue on that, not immediately though.

Requiring packages to have access to internet during build step can make them vulnerable to supply chain attacks, in addition to not being able to make builds reproducible (e.g. for audits).

It is not a requirement but a convenience option. Currently user-provided artifacts are preferred over downloading one.

The end goal is so that all that logic for downloading artifacts done in CMake is removed and instead use ansible (or instruct the user to download the artifacts manually).

Ok, if there has been a definitive decision to remove the cmake downloads from all packages for whatever reason I can live with that. But can we agree it isn't justified by any of the points above?

[codecov]

Codecov Report

Attention: 23 lines in your changes are missing coverage. Please review.

Comparison is base (fdd671f) 14.73% compared to head (3c97359) 14.78%.
Report is 9 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4923      +/-   ##
==========================================
+ Coverage   14.73%   14.78%   +0.04%     
==========================================
  Files        1661     1663       +2     
  Lines      115531   115633     +102     
  Branches    35675    35735      +60     
==========================================
+ Hits        17023    17093      +70     
+ Misses      79272    79237      -35     
- Partials    19236    19303      +67     

Flag	Coverage Δ		*Carryforward flag
differential	39.31% <50.00%> (?)
total	14.75% <ø> (+0.02%)	⬆️	Carriedforward from d080050

*This pull request uses carry forward flags. Click here to find out more.

Files	Coverage Δ
...king/include/autonomous_emergency_braking/node.hpp	0.00% <ø> (ø)
control/autonomous_emergency_braking/src/node.cpp	0.00% <ø> (ø)
...ommon/tvm_utility/include/tvm_utility/pipeline.hpp	31.57% <20.00%> (ø)
common/tvm_utility/test/abs_model/main.cpp	53.65% <53.65%> (ø)

... and 10 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lexavtanke]

I've updated tvm_utils. As example for other tvm based packages. Introduced new constructor for InferenceEngineTVM with additional parameter autoware_data_path. So package expect to find artifacts for inference (as deploy_graph.json, deploy_lib.so, deploy_param.params) in autoware_data/tvm_utility/ with the same structure as it was in tvm_utility/data folder.

Back compatible with other tvm packages as previous constructor is kept in place

[lexavtanke]

@ambroise-arm Can you please elaborate a bit how tvm_utility should be reworked:

I had started to rework the way tvm_utility worked to allow to remove the need for the artifact to be present at compile time, but I didn't see much interest in TVM packages so I parked the work.

So we can consider such option as one possible solution.

[ambroise-arm]

@lexavtanke At a high level InferenceEngineTVM needs to have its constructor interface changed so that it requires getting the network name and paths to the files to be dynamically loaded at runtime (the .so, .json and .params) instead of reading it from the artifact header file. And the remaining parameters from that header file need to be extracted from the network at runtime (with TVM's runtime_mod.GetFunction(...)). So that there is no dependency on the artifacts at build time.

All the tvm packages in autoware need to be changed to use the new interface. The path to those .so, .json and .params need to become parameters of the nodes making use of tvm_utilty. That probably needs to be integrated with the new way of getting the artifacts you were talking about, in order to make it easy for the user to select a specific backend, and potentially switching between them without too much effort (and also user-provided artifacts).

And then there can be optimizations that can be done to https://github.com/autowarefoundation/modelzoo/ in order to stop generating that header file that won't be used anymore.

[ambroise-arm]

nit: This file can be removed if it's empty.

[esteve]

@lexavtanke LGTM, thanks for the patience iterating with us 🙂

[esteve]

@angry-crab could you approve this PR when you have a moment? You're one of the codeowners of this package, I approved these changes, but they won't merge unless a codeowner approves it as well. Thanks!

[ambroise-arm]

Am I not a codeowner of tvm_utility?

[esteve]

@ambroise-arm probably, but only @angry-crab had the codeowner logo, I assumed he was the only one. Anyway, @ambroise-arm thanks for approving this PR