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

Move downloading artifacts outside CMake #3137

Closed
38 tasks done
Tracked by #3222
esteve opened this issue Mar 22, 2023 · 14 comments · Fixed by autowarefoundation/autoware#3375 or autowarefoundation/autoware-documentation#345
Closed
38 tasks done
Tracked by #3222
Assignees
Labels
type:new-feature New functionalities or additions, feature requests.

Comments

@esteve
Copy link
Contributor

esteve commented Mar 22, 2023

Checklist

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

Description

Some packages download artifacts by default, which is not necessary for compiling them. Additionally, during the process for generating Debian packages, access to the network is restricted, so building these packages fail (see autowarefoundation/autoware#3222 (comment))

The packages that download ONNX models are:

Other types of artifacts:

Update ansible scripts to download artifacts files:

Update documentation with instructions on how to download ONNX files:

Disable internet access for CI actions:

Add data_path variable to autoware_launch:

Update default path to artifacts in package launch files:

Purpose

Be able to build the packages without requiring to download the ONNX models

Possible approaches

Remove the download logic so that ONNX models, update ansible scripts to download them when deploying the packages and document how to manually download the models.

Definition of done

Packages can be built without downloading the ONNX models. Documentation is updated to let users manually download the ONNX models. Update ansible scripts to download the models.

@yukkysaito
Copy link
Contributor

Thank you for your proposal.
If DOWNLOAD_ARTIFACTS is default false, won't the rosbag replay simulator stop working?

@esteve
Copy link
Contributor Author

esteve commented Mar 23, 2023

@yukkysaito thanks for your comment. I've only submitted changes to the packages that download ONNX models. Could you link to the packages that download rosbag files? Thanks.

In any case, the reason for not downloading external files by default is because the processes we use for generating Debian packages restrict access to the network (as is the Debian policy for building packages), so we can't download files in CMakeLists.txt. I have some questions:

  • Is there another way to distribute the rosbag files?
  • Perhaps a separate package or adding instructions for users?
  • How big are the files?
  • Maybe we could use something like git-lfs to store them in Git?

Thanks.

@yukkysaito
Copy link
Contributor

@esteve sorry, I may have written it badly and caused a misunderstanding.
I am not talking about rosbag, but rosbag replay simulator. 🙏
https://autowarefoundation.github.io/autoware-documentation/main/tutorials/ad-hoc-simulation/rosbag-replay-simulation/

rosbag replay simulator uses DNN such as centerpoint, so it is necessary to download the onnx file.
If the default is set to false, the onnx file will not be downloaded and the rosbag replay simulator in the tutorial will not work.

I would preferably download it when setting up the environment in ansible, so that no internet connection is needed at build time.

@esteve
Copy link
Contributor Author

esteve commented Mar 23, 2023

@yukkysaito ah, sorry, I misunderstood. Thanks, I agree it'd be much easier if downloading the ONNX models is done outside the build process of CMakeLists.txt. I can remove the logic from CMakeLists.txt and we can update the ansible scripts. I'm not familiar with ansible, but I can try.

@xmfcx
Copy link
Contributor

xmfcx commented Mar 24, 2023

I agree, the links are in the CMakeLists.txt file and most are in https://web.auto domain name right now.

Moving them to ansible and also adding the manual steps in the readme of the role should be enough.

And yes like @esteve suggested we should remove the remains of the downloads from the CMakeLists.txt files.

So we should remove the DOWNLOAD_ARTIFACTS in general. (But if you want to add the logic and set the default to false before removing it, it's fine too)

@BonoloAWF BonoloAWF added the type:new-feature New functionalities or additions, feature requests. label Mar 24, 2023
@BonoloAWF BonoloAWF added this to the Mar - Apr Milestone milestone Mar 24, 2023
@esteve
Copy link
Contributor Author

esteve commented Mar 24, 2023

Following this discussion I've closed autowarefoundation/autoware_common#164 and will rework the rest of the PRs to remove the download logic. I'll also update the documentation and the ansible scripts.

@esteve esteve changed the title Disable downloading ONNX models by default Move downloading ONNX outside CMake Mar 24, 2023
@esteve esteve self-assigned this Mar 24, 2023
@yukkysaito
Copy link
Contributor

There are a few things to consider for downloading with ansible.

  • Where to put the downloads?
  • How do DNN pkgs (centerpoint, etc.) refer to the downloaded file from launch?

As an example,to refer to the downloaded file in launch file by ansible, we can create a specific package, put the weight file there, and refer to it from launch.
Do you have any other suggestions?

@xmfcx
Copy link
Contributor

xmfcx commented Mar 28, 2023

@yukkysaito

Right now Autoware has map_path as an external directory necessary for the runtime:

ros2 launch autoware_launch logging_simulator.launch.xml \
map_path:=$HOME/autoware_map/sample-map-rosbag \
vehicle_model:=sample_vehicle \
sensor_model:=sample_sensor_kit

What about adding a ml_models_path (machine learning) path too?

Then this path could be passed to the necessary nodes.

@esteve
Copy link
Contributor Author

esteve commented Mar 28, 2023

There are a few things to consider for downloading with ansible.

* Where to put the downloads?

* How do DNN pkgs (centerpoint, etc.) refer to the downloaded file from launch?

As an example,to refer to the downloaded file in launch file by ansible, we can create a specific package, put the weight file there, and refer to it from launch. Do you have any other suggestions?

Sorry, I'm afraid I'm not following here. Will the package store the weight file? And if not using ansible, how will the package get populated with the model files? Could you explain further? Thanks.

@esteve
Copy link
Contributor Author

esteve commented Mar 28, 2023

@xmfcx

What about adding a ml_models_path (machine learning) path too?

Then this path could be passed to the necessary nodes.

I think this is a good idea, it'll standardize how to access the models across all nodes. What path by default should we tell users to download files to? With ansible we can decide for them, but I'm thinking of when Autoware is installed manually.

Also, when/if we generate Debian packages for these packages, should the models be installed system-wide or in a user's folder?

@yukkysaito
Copy link
Contributor

yukkysaito commented Mar 30, 2023

What about adding a ml_models_path (machine learning) path too?
Then this path could be passed to the necessary nodes.

@xmfcx @mitsudome-r
I think the map path is necessary because it specifies the environment in which to run, but ml_model_path seems to be a parameter of convenience inside Autoware.
So I would like to make the user don't care it, is there a good way to do this?

@esteve esteve changed the title Move downloading ONNX outside CMake Move downloading artifacts outside CMake Jun 30, 2023
@stale
Copy link

stale bot commented Aug 29, 2023

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

@stale stale bot removed the status:stale Inactive or outdated issues. (auto-assigned) label Sep 5, 2023
@esteve esteve reopened this Sep 19, 2023
@esteve
Copy link
Contributor Author

esteve commented Nov 10, 2023

Closing this ticket now that all the linked PRs are merged. Thanks everyone, especially @lexavtanke

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment