ci: add sync-params workflow #1812
ci: add sync-params workflow #1812paulsohn wants to merge 51 commits intoautowarefoundation:mainfrom
Conversation
Signed-off-by: Taeseung Sohn <taeseung.sohn@tier4.jp>
Signed-off-by: Taeseung Sohn <taeseung.sohn@tier4.jp>
Signed-off-by: Taeseung Sohn <taeseung.sohn@tier4.jp>
Signed-off-by: Taeseung Sohn <taeseung.sohn@tier4.jp>
Signed-off-by: Taeseung Sohn <taeseung.sohn@tier4.jp>
|
Thank you for contributing to the Autoware project! 🚧 If your pull request is in progress, switch it to draft mode. Please ensure:
|
Signed-off-by: Taeseung Sohn <taeseung.sohn@tier4.jp>
fac6446 to
6db6848
Compare
| rev: v4.0.0-alpha.8 | ||
| hooks: | ||
| - id: prettier | ||
| args: [--no-error-on-unmatched-pattern] |
There was a problem hiding this comment.
memo: this aligns with autowarefoundation/sync-file-templates#56 .
|
@paulsohn This action looks very useful. I'm not sure if this is possible, but how about saving the original parameters as comments, like - some_list: [10, 20] # OVERRIDE: [1, 2]
+ some_list: [10, 20] # OVERRIDE: [1, 2, 3, 4] |
Signed-off-by: Taeseung Sohn <taeseung.sohn@tier4.jp>
Signed-off-by: Taeseung Sohn <taeseung.sohn@tier4.jp>
Signed-off-by: Taeseung Sohn <taeseung.sohn@tier4.jp>
Signed-off-by: Taeseung Sohn <taeseung.sohn@tier4.jp>
Signed-off-by: Taeseung Sohn <taeseung.sohn@tier4.jp>
Signed-off-by: Taeseung Sohn <taeseung.sohn@tier4.jp>
Signed-off-by: Taeseung Sohn <taeseung.sohn@tier4.jp>
Signed-off-by: Taeseung Sohn <taeseung.sohn@tier4.jp>
Signed-off-by: Taeseung Sohn <taeseung.sohn@tier4.jp>
Signed-off-by: Taeseung Sohn <taeseung.sohn@tier4.jp>
Signed-off-by: Taeseung Sohn <taeseung.sohn@tier4.jp>
Signed-off-by: Junya Sasaki <junya.sasaki@tier4.jp>
|
@paulsohn I reviewed a source code and put my understanding as this commit. Let me consider a way about this @isamu-takagi's proposal. |
Signed-off-by: Taeseung Sohn <taeseung.sohn@tier4.jp>
Signed-off-by: Taeseung Sohn <taeseung.sohn@tier4.jp>
|
I have seriously reviewed three plausible options:
And still after the comparison I concluded that full footer solution is genuinely a better choice if we consider maintenance cost as well. Imagine a downstream maintainer wants to override a field which was not previously overridden. If we choose full footer, they can just add a comment
If we inline the shadowed value marker comments then at least we don't have that pivot line, but the responsibility for providing correct marker format is still on the maintainer and we still are facing the matrix issue. So the maintenance cost makes full footer as an optimal choice, rather than a non-ideal but easy-to-implement workaround. One more note: the integration-side maintainer needs to have the full parameter actually applied to the runtime and the original values of overridden fields, not only overriding values. Integration engineers need to understand the upstream intent transparently in case the upstream parameter format or required fields has changed unexpectedly. EDIT: the final consent has been made to have no footer, but I will leave this comment for the record. |
|
Notes from Software WG: We will make the copy of original parameters optional and set it false for Autoware Launch repo. |
Signed-off-by: Taeseung Sohn <taeseung.sohn@tier4.jp>
Signed-off-by: Taeseung Sohn <taeseung.sohn@tier4.jp>
Signed-off-by: Taeseung Sohn <taeseung.sohn@tier4.jp>
Signed-off-by: Taeseung Sohn <taeseung.sohn@tier4.jp>
Signed-off-by: Taeseung Sohn <taeseung.sohn@tier4.jp>
Signed-off-by: Taeseung Sohn <taeseung.sohn@tier4.jp>
|
At 6242fa5 I confirmed in my fork that this sync-params workflow gives the expected PR with information of shadowed changes by override. Upstream footers removed, and appending footer is now spun out as |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 44 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if repo_dir.exists(): | ||
| shutil.rmtree(repo_dir) | ||
| # Full history is required to compute per-file last-modified SHA accurately. | ||
| run_git(["clone", "--branch", ref, f"https://github.com/{repo}.git", str(repo_dir)]) |
There was a problem hiding this comment.
clone_repository() uses git clone --branch <ref> ..., which fails when ref is a raw commit SHA. The script/docstring and config comment indicate ref may be a branch/tag/SHA, so this will break valid configurations. Consider cloning without --branch (or using --no-checkout + git fetch), then git checkout <ref> so SHA pins work reliably.
| run_git(["clone", "--branch", ref, f"https://github.com/{repo}.git", str(repo_dir)]) | |
| # Do not use ``git clone --branch`` here: it accepts branch/tag names but not | |
| # raw commit SHAs, while ``ref`` is documented to support branch/tag/SHA. | |
| run_git(["clone", f"https://github.com/{repo}.git", str(repo_dir)]) |
There was a problem hiding this comment.
ref is not expected to be a fixed SHA. The whole point of this workflow is to track a moving ref, so branch is expected. Skipping.
Signed-off-by: Taeseung Sohn <taeseung.sohn@tier4.jp>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 44 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@xmfcx @mitsudome-r
|
Signed-off-by: Taeseung Sohn <taeseung.sohn@tier4.jp>
2ba0f91 to
f217483
Compare


Description
Tracking issue: autowarefoundation/autoware#7048
Introduce
sync-paramsworkflow that updates configurated*.param.yamlwith custom overrides.As described in
sync-params.yamlin this PR, the configuration goes like this.In this case, the python run with category
python3 ./.github/scripts/sync_params.py perceptionclones the upstream repository to a temp directory, copies source(autoware_universe)/perception/autoware_image_object_locator/config/bbox_object_locator.param.yamlto each variant path - in this case only(autoware_launch)/autoware_launch/config/perception/object_recognition/detection/camera_vru_detection/near_range_camera_vru_detector.param.yamlwith a comment header.and duplicates the original file content as a comment block at the bottom of the fileIn variant files, leaf fields (i.e. scalar or list of scalars) can be marked overridden by having a comment starting with
{OVERRIDE}or{OVERRIDE: reason to override}(the reason can be any text without newlines or braces). In this case, the marked leaf field is overlaid upon the original. So if you want to override a certain value, add# {OVERRIDE}or# {OVERRIDE: reason}comment (or prepend the existing one with it).Example
suppose the upstream param file looks like:
and the corresponding downstream variant:
now suppose the upstream param file changes:
then after syncing, the downstream will be:
Check mode
The check-params workflow runs
sync_params.py --check <category>on every PR. In check mode the script performs the same computation as update mode - cloning upstream, applying overrides, building the expected variant content - but writes nothing to disk, and exits non-zero if any variant has drifted from its expected state.Check mode uses the SHA pinned in each variant's header (the
# Source:line) rather than the current upstream HEAD. Upstream changes between the pinned SHA and HEAD are intentionally ignored, because those are the responsibility ofupdate-params. A freshly synced variant will always pass check mode until someone edits it by hand without going through the sync workflow.Rationales on designs
Q. Why do we have categories instead of bumping all param files at once?
perception / localization / etc have different codeowners, so one large PR updating all param files across all categories will need many approvals to sync. The exact granularity is a subject to change (e.g. we can have
perception/object_recognitionandperception/predictioncategories split instead of one largeperceptioncategory) but it is beyond the scope of this PR.Q. Why shouldn't we only annotate the overridden value with the original one?
In the recent SW WG we decided not to keep upstream param file as footer, while my original suggestion was having a full copy footer. So the choice was all or none - not selectively marking overridden fields.
Maintaining 'shadowed value' marker has two limitations: one is the technical limitation of ruamel and the other is maintenance cost - if a maintainer should override a previously not overridden field, the maintainer should know the exact format of the shadowed value marker, and this can be tricky to define when some fields are actually matrices that span across multiple lines.
In our current implementation we can have only
# {OVERRIDE}marker, which is always one-liner and much simpler.My argument supporting having full footer than no footer (unaccepted for AWF)
This is for visibility of updates of overridden leaf fields.
We can of course sync upstream source to variants directly, without duplicating the original as a comment footer. However when the upstream changes a leaf field, then the syncing PR might be ignorant to the context of the upstream change because the diff is suppressed by overriding mechanism.
For example, if the upstream param file has a field
and the downstream overrides it
and later the upstream changes the field to
some_list: [1,2,3,4]because the list size should be four, the sync PR will be signaled that the upstream has been changed, but that exact diff is suppressed so the reviewer might not realize or forget that the override value should be updated to length-4 list e.g.[10, 20, 30, 40]. Having a local copy on this repository exposes the exact diff to the sync PR.Since the header contains the last SHA that upstream file has changed, we can actually get notified when it has changed, but to see what has been changed, we need to open the link and see the upstream file history.
For real-world integration (e.g. products in my company) I am considering to enable
--footeroption to have full footer, just as my original suggestion before AWF discussion.TODOs before getting approval of this PR
python3 .github/scripts/sync_params.py --check perceptionfor this.Future works beyond the scope of this PR
How was this PR tested?
Notes for reviewers
Effects on system behavior
None.