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

feat: use ansible to download ONNX files #3375

Merged
merged 11 commits into from
Sep 19, 2023
Merged

feat: use ansible to download ONNX files #3375

merged 11 commits into from
Sep 19, 2023

Conversation

esteve
Copy link
Contributor

@esteve esteve commented Mar 28, 2023

Description

This PR uses Ansible to download ONNX files.

Fixes autowarefoundation/autoware.universe#3137

Pre-review checklist for the PR author

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

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

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
Copy link
Contributor Author

esteve commented Mar 28, 2023

@kenji-miyake I don't have much experience with Ansible, could you please have a look to make sure I'm on the right path? I'll add more tasks for more files. Thanks 🙏

@esteve esteve marked this pull request as ready for review July 12, 2023 10:22
@esteve
Copy link
Contributor Author

esteve commented Jul 12, 2023

@xmfcx @mitsudome-r this PR is now ready for review

@xmfcx
Copy link
Contributor

xmfcx commented Sep 5, 2023

@xmfcx
Copy link
Contributor

xmfcx commented Sep 5, 2023

Let the user pass the "autoware_data" folder directory to the ansible script. (ref: https://github.com/autowarefoundation/autoware/blob/main/setup-dev-env.sh ) (default: ~/autoware_data/ as suggested by @esteve )
Then ONNX files will be downloaded into:

.../autoware_data/package_name/map.pcd
.../autoware_data/package_name/map.josm ?
.../autoware_data/package_name/model1.onnx

Details in: https://github.com/orgs/autowarefoundation/discussions/3649#discussioncomment-6475228

Once this PR is tested and merged,

We can modify the launch files to point to these new files.

And we can proceed to remove existing cmake references.

@esteve esteve marked this pull request as draft September 7, 2023 11:20
@esteve esteve marked this pull request as ready for review September 8, 2023 14:53
@esteve
Copy link
Contributor Author

esteve commented Sep 8, 2023

@xmfcx this PR is ready for review now, I've added an option to specify the directory to download the files to and updated the ansible scripts to cover all the artifacts.

@xmfcx
Copy link
Contributor

xmfcx commented Sep 11, 2023

@lexavtanke could you review this please? If you have any questions, please don't hesitate to ask me or @esteve 🙏

@lexavtanke
Copy link
Contributor

Downloading part works good for ONNX files. But I have mention that it can be confusing for the user that he need to create the structure inside the autoware_data folder by itself. May be it is better to add it to the ansible script. Cause then people use it for preparing the env in is a bit annoying then you need to start process again and again after you meet the errors like this
Screenshot from 2023-09-12 00-54-51
Screenshot from 2023-09-12 00-31-18

I also checked updates in documentation and didn't find the folder preparation in the current version.

@esteve
Copy link
Contributor Author

esteve commented Sep 15, 2023

@lexavtanke and I discussed this over Discord in private, I agree that this PR needs to be updated so that the folders for each package are created before downloading the models, ansible does not create folders automatically and needs to be told to create them beforehand.

@esteve esteve marked this pull request as draft September 15, 2023 07:50
@esteve esteve marked this pull request as ready for review September 15, 2023 09:06
@esteve esteve enabled auto-merge (squash) September 15, 2023 09:06
@esteve
Copy link
Contributor Author

esteve commented Sep 15, 2023

@lexavtanke I've updated the PR to make sure that the directories are created per package before downloading the models, can you have another look? Thanks.

@lexavtanke
Copy link
Contributor

@esteve Everything works good with creating folders but I experienced weird things during downloading some files:

Screenshot from 2023-09-15 15-31-01
Screenshot from 2023-09-15 15-10-06
Some checksums don't match in my case. I made suggestions in the review.

And now I noticed that ansible role named onnx. I think it can be confusing if it will be used to download artifacts for other packages. May be it is worth to change it to something like get_artfacts or just artifacts. What do you think?

@esteve
Copy link
Contributor Author

esteve commented Sep 15, 2023

@lexavtanke

May be it is worth to change it to something like get_artfacts or just artifacts. What do you think?

Good idea, artifacts sounds good to me.

Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
esteve and others added 5 commits September 18, 2023 14:23
Co-authored-by: Alexey Panferov <37497658+lexavtanke@users.noreply.github.com>
Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
@esteve
Copy link
Contributor Author

esteve commented Sep 18, 2023

@lexavtanke I've fixed all the ansible lint issues, could you have a look this PR again? Thanks.

Copy link
Contributor

@lexavtanke lexavtanke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@esteve LGTM, thank you.

@lexavtanke
Copy link
Contributor

@xmfcx I guess we need approval from you. Thanks

@esteve esteve merged commit f768b2c into autowarefoundation:main Sep 19, 2023
16 checks passed
@esteve esteve deleted the update-ansible-onnx branch September 19, 2023 11:07
@esteve
Copy link
Contributor Author

esteve commented Sep 19, 2023

@xmfcx thanks 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move downloading artifacts outside CMake
3 participants