[NPU][CI] Refactor patch reconciliation - #390
Conversation
Signed-off-by: Meihan-chen <zr010426ztt@outlook.com>
Documentation build overview
48 files changed ·
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the NPU patch management system in the CI pipeline, introducing a runtime patch reconciler that dynamically applies and reverts patches based on a declared series configuration (series.conf). The update script has been rewritten to calculate patch series digests and reconcile changes, while the Dockerfile now persists the patch state. The review feedback suggests several shell script improvements, including initializing global variables earlier to avoid stale states, removing redundant dry-run git apply --check operations, and wrapping the main function execution to prevent it from running when the script is sourced.
| if [ ! -f "$series_file" ]; then | ||
| echo "ERROR: Patch series not found: $series_file" >&2 | ||
| return 1 | ||
| fi | ||
| } | ||
|
|
||
| revert_patch() { | ||
| local component_dir="$1" | ||
| local patch_name="$2" | ||
| local old_patch_path="$3" | ||
|
|
||
| if [ -f "$old_patch_path" ]; then | ||
| echo "INFO: Attempting to reverse-apply old patch from $old_patch_path" | ||
| if git -C "$component_dir" apply --reverse --whitespace=nowarn "$old_patch_path"; then | ||
| echo "INFO: Successfully reverted old $patch_name" | ||
| else | ||
| echo "WARNING: Failed to reverse-apply old patch $patch_name, skipping" | ||
| SERIES_ENTRIES=() |
There was a problem hiding this comment.
Initializing SERIES_ENTRIES after the file existence check can leave the global array in a stale state if the function returns early. It is safer to clear the array at the very beginning of the function to ensure a clean state on failure.
| if [ ! -f "$series_file" ]; then | |
| echo "ERROR: Patch series not found: $series_file" >&2 | |
| return 1 | |
| fi | |
| } | |
| revert_patch() { | |
| local component_dir="$1" | |
| local patch_name="$2" | |
| local old_patch_path="$3" | |
| if [ -f "$old_patch_path" ]; then | |
| echo "INFO: Attempting to reverse-apply old patch from $old_patch_path" | |
| if git -C "$component_dir" apply --reverse --whitespace=nowarn "$old_patch_path"; then | |
| echo "INFO: Successfully reverted old $patch_name" | |
| else | |
| echo "WARNING: Failed to reverse-apply old patch $patch_name, skipping" | |
| SERIES_ENTRIES=() | |
| SERIES_ENTRIES=() | |
| if [ ! -f "$series_file" ]; then | |
| echo "ERROR: Patch series not found: $series_file" >&2 | |
| return 1 | |
| fi |
| git -C "$target" apply --check --whitespace=nowarn "$patch_path" | ||
| git -C "$target" apply --whitespace=nowarn "$patch_path" |
There was a problem hiding this comment.
Running git apply --check followed immediately by git apply on the same patch in the same loop iteration is redundant. git apply is atomic and will fail cleanly if the patch cannot be applied. Removing the dry-run check simplifies the code and avoids running the patch application logic twice.
| git -C "$target" apply --check --whitespace=nowarn "$patch_path" | |
| git -C "$target" apply --whitespace=nowarn "$patch_path" | |
| git -C "$target" apply --whitespace=nowarn "$patch_path" |
| git -C "$target" apply --reverse --check --whitespace=nowarn "$patch_path" | ||
| git -C "$target" apply --reverse --whitespace=nowarn "$patch_path" |
There was a problem hiding this comment.
Similarly to the apply step, running git apply --reverse --check followed immediately by git apply --reverse is redundant. Removing the dry-run check simplifies the code.
| git -C "$target" apply --reverse --check --whitespace=nowarn "$patch_path" | |
| git -C "$target" apply --reverse --whitespace=nowarn "$patch_path" | |
| git -C "$target" apply --reverse --whitespace=nowarn "$patch_path" |
| if [[ "${BASH_SOURCE[0]}" == "$0" ]]; then | ||
| case "${1:-}" in | ||
| series-digest) | ||
| if [ "$#" -ne 3 ]; then | ||
| echo "Usage: $0 series-digest SERIES_FILE SOURCE_ROOT" >&2 | ||
| exit 2 | ||
| fi | ||
| series_digest "$2" "$3" | ||
| exit | ||
| ;; | ||
| reconcile) | ||
| if [ "$#" -ne 5 ]; then | ||
| echo "Usage: $0 reconcile OLD_SERIES OLD_ROOT NEW_SERIES NEW_ROOT" >&2 | ||
| exit 2 | ||
| fi | ||
| reconcile_series "$2" "$3" "$4" "$5" | ||
| exit | ||
| ;; | ||
| esac | ||
| fi | ||
|
|
||
| main |
There was a problem hiding this comment.
Executing main at the global scope means it will run automatically when the script is sourced (e.g., for testing or reusing helper functions). Wrapping main inside the if [[ "${BASH_SOURCE[0]}" == "$0" ]] block as the default case ensures it only runs when the script is executed directly.
if [[ "${BASH_SOURCE[0]}" == "$0" ]]; then
case "${1:-}" in
series-digest)
if [ "$#" -ne 3 ]; then
echo "Usage: $0 series-digest SERIES_FILE SOURCE_ROOT" >&2
exit 2
fi
series_digest "$2" "$3"
exit
;;
reconcile)
if [ "$#" -ne 5 ]; then
echo "Usage: $0 reconcile OLD_SERIES OLD_ROOT NEW_SERIES NEW_ROOT" >&2
exit 2
fi
reconcile_series "$2" "$3" "$4" "$5"
exit
;;
*)
main
;;
esac
fiSigned-off-by: Meihan-chen <zr010426ztt@outlook.com>
e619341 to
44773c0
Compare
5e548d6 to
97134d2
Compare
Signed-off-by: Meihan-chen <zr010426ztt@outlook.com>
97134d2 to
58aa103
Compare
0b0170f to
80832d8
Compare
Signed-off-by: Meihan-chen <zr010426ztt@outlook.com>
2 participants
Summary
series.conf/opt/npu_patchimage-buildis requiredsmksuiteMotivation
The previous updater used the current checkout's static
PATCH_CONFIGSto represent both the image's OLD patch state and the requested NEW state. This is not reliable when patches are deleted, renamed, reordered, or modified.PR #385 exposed this issue in NPU CI build 167.
The updater did not retain the deleted
megatron_comm.patchandmindspeed.patch, failed to revert the image's Megatron patch, and then tried to apply the new patch on top of the remaining OLD state.Design
The image stores its ordered OLD series and exact patch bytes under
/opt/npu_patch. The current Vime checkout provides the NEW series throughdocker/npu_patch/series.conf.Each entry uses:
If the OLD and NEW digests differ, CI reverts the OLD series from bottom to top and applies the NEW series from top to bottom. This allows the image to revert patches that have already been removed from the current checkout.
Contributor impact
Patch changes remain explicit and reviewable. Contributors must update the patch file, the corresponding
docker/Dockerfile.npuapply operation, anddocker/npu_patch/series.confin the same change. Ordinary patch lifecycle changes no longer require patch-specific CI logic.