feat(autoware_path_optimizer): reintroducing acados MPT along with changes to linking to acados#12300
Conversation
|
Thank you for contributing to the Autoware project! 🚧 If your pull request is in progress, switch it to draft mode. Please ensure:
|
soblin
left a comment
There was a problem hiding this comment.
Hope this fix solves the issue !
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12300 +/- ##
==========================================
+ Coverage 18.48% 19.19% +0.71%
==========================================
Files 1849 1879 +30
Lines 127898 129128 +1230
Branches 45490 47955 +2465
==========================================
+ Hits 23638 24783 +1145
- Misses 84639 85621 +982
+ Partials 19621 18724 -897
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Not directly related issue, but since you opened a follow-up, I would like to have one more change before we need another PR:
This is the clean Jazzy CI environment from my personal research purpose, unrelated to AWF. Building this package (with symlink install) overwrites the existing placeholder file, making the repository status dirty. My local build reports the same.
Why did we decided to put the placeholder file at the first place? I recall it was about clang-tidy check, but is it really necessary? I couldn't track on the relevant discussion in the previous PR.
As a solution I want you to investigate on whether either of them is feasible.
- remove the placeholder file, or
- put placeholder file in another directory and let cmake copy when build
|
@PanConChicharron I would say that the fix is essential to prevent accidental commit updating placeholder. |
You are right, and I should definitely figure out an alternative. I meant Github CI, and unfortunately it seems to run before the file is generated, which causes clang-tidy to fail. CC: @mitsudome-r @xmfcx |
ec59685 to
167ca99
Compare
|
Committed the generated files as a last resort. |
ce847a7 to
6d902ba
Compare
| -o acados_ocp_solver_pyx.c \ | ||
| -I $(INCLUDE_PATH)/../interfaces/acados_template/acados_template \ | ||
| $(INCLUDE_PATH)/../interfaces/acados_template/acados_template/acados_ocp_solver_pyx.pyx \ | ||
| -I /home/arjunram/Workspace/autoware/src/universe/autoware_universe/planning/autoware_path_optimizer/src/acados_mpc/c_generated_code \ |
There was a problem hiding this comment.
| -I /home/arjunram/Workspace/autoware/src/universe/autoware_universe/planning/autoware_path_optimizer/src/acados_mpc/c_generated_code \ |
This has hardcoded user path. We cannot include this kind of auto-generated code in the src folder.
Also because of pre-commit, after each colcon build, we have 15 changes due to code layout refresh. Have to run pre-commit again to reduce them to 1 change. (which is the makefile)
Is it possible to install these auto-generated codes into the install folder and refer to them from there?
There was a problem hiding this comment.
This is the main issue to be dealt with now.
Unfortunately, it's not clear how to proceed. We need autoware_universe/planning/autoware_path_optimizer/src/acados_mpc/c_generated_code/acados_solver_curvilinear_bicycle_model_spatial.h because clang-tidy will fail without it.
Adding a placeholder yields modifications to that file when the code is actually generated, which makes it harder on developers.
And adding the generated code explictly, brings up the above issue (worth noting that changing the code as you had suggested will introduce diffs in every user's workspace after code-generation).
There was a problem hiding this comment.
what about putting auto-generated code into the install folder?
There was a problem hiding this comment.
Hmm... That could be done, but then I would need some really messy relative directories in the code to point to the install folder.
Instead, I could 'git restore' the code at the end of the CMakeLists.
|
git commands like https://cmake.org/cmake/help/book/mastering-cmake/chapter/Getting%20Started.html#directory-structure
https://crascit.com/2017/04/18/generated-sources-in-cmake-builds/ Generating source code into the source folder is the root cause of all these issues, let's fix that first. Following post is ai genereated 🤖 but worth trying. Code Generation Should Target the Binary Directory, Not the Source Tree 🤖Hey @PanConChicharron — I took a closer look at the CMake changes and I think the current approach introduces a number of issues that will bite contributors and CI down the line. I want to lay out the problems clearly and then propose a concrete alternative that solves the original clang-tidy issue without the trade-offs. Problems with the Current Approach1. The
|
| Concern | Current approach | Proposed approach |
|---|---|---|
| Incremental builds | Broken — codegen reruns every time | Works correctly via timestamp tracking |
| Source tree cleanliness | Requires git checkout hack |
Source tree is never touched |
| Read-only source dirs | Build fails | Works out of the box |
git dependency at build time |
Required | Not needed |
| clang-tidy support | Placeholder may not satisfy analysis | Minimal stub with real prototypes |
| Lint exclusions | Glob workarounds needed | Not needed |
| Build from tarball | Fails | Works |
The entire complexity in the current implementation — in-tree generation, git restore, lint exclusion globs — stems from one decision: generating into the source tree. Moving that to the binary dir eliminates all of it, and the add_custom_command / add_custom_target / add_dependencies chain you already have is exactly right — it's just pointed at the wrong directory.
… to failing builds (autowarefoundation#12298)" This reverts commit 7302e8c. Signed-off-by: Arjun Jagdish Ram <arjun.ram@tier4.jp>
Signed-off-by: Arjun Jagdish Ram <arjun.ram@tier4.jp>
Signed-off-by: Arjun Jagdish Ram <arjun.ram@tier4.jp>
Signed-off-by: Arjun Jagdish Ram <arjun.ram@tier4.jp>
Signed-off-by: Arjun Jagdish Ram <arjun.ram@tier4.jp>
Signed-off-by: Arjun Jagdish Ram <arjun.ram@tier4.jp>
Signed-off-by: Arjun Jagdish Ram <arjun.ram@tier4.jp>
Signed-off-by: Arjun Jagdish Ram <arjun.ram@tier4.jp>
Signed-off-by: Arjun Jagdish Ram <arjun.ram@tier4.jp>
Signed-off-by: Arjun Jagdish Ram <arjun.ram@tier4.jp>
This reverts commit 6496b40.
This reverts commit 8261580.
Signed-off-by: Arjun Jagdish Ram <arjun.ram@tier4.jp>
a2ad8db to
a952ec6
Compare
Fatih, thank you for the very detailed explanation! I went ahead and made the changes as per your request. Just waiting for CI now. |

Description
Follow-up to #11479 to propagate changes downstream.
Related links
Parent Issue:
Ticket
How was this PR tested?
Evalautor build succesful: https://evaluation.tier4.jp/evaluation/reports/8a01ff9b-5732-5a87-abd5-931e6d93c6a7?project_id=autoware_dev (with pilot-auto PR: https://github.com/tier4/pilot-auto/pull/2389)
Notes for reviewers
None.
Interface changes
None.
Effects on system behavior
None.