Added incremental stamp/hash check for aiter checkout - #5
Conversation
* Initial implementation of floating commit * Updated to use only transient checkout of AITER (gitignored) * Update to patch-based build * Updated transient checkout staging area * Offset patch * Include third_party spec for AITER API * Add temporary CK patch * Added QoLA checkout CLI command * Updated patch * Added apply non-3way bypass for CK submodule patches * Rebased patches * Added gfx-1250 changes * Updated fwd path w/ stream guard * Cherry-pick from Disentangle AITER subcommit from QoLA commit (#5) * Add skip checkout option * Let override hsa_sbdirs * Added entrypoint for workspace calc * Updated guards for CK-free build * Add AITER top-left mask to bottom-right patch * Add device-side workspace prep kernel * Update zeroing policy for dq_acc * Corrected memory zeroing from AITER side * Corrected patch --------- Co-authored-by: Ilya Panfilov <ipanfilo@amd.com>
ipanfilo
left a comment
ipanfilo
left a comment
There was a problem hiding this comment.
The proposed change will skip re-sync even if aiter tree manually updated after writing stamp key. On one hand it it is flaw that does not guarantee tree state. On the other hand it gives developer flexibility to run build with local AITER changes. What is desired goal of thi change?
Main intent is to enable a more generalized caching of build for TE's cmake build system to be able to skip redundant checkouts. The ability to mutate without forcing a new checkout is something that can be mediated via |
| tracked, patched, or clobbered by ``git reset``/``git submodule update``. | ||
| ``git rev-parse --absolute-git-dir`` resolves both a real ``.git`` | ||
| directory (fresh clone) and a ``.git`` file pointing elsewhere (worktree | ||
| or submodule-style checkout). |
There was a problem hiding this comment.
I'd say it is disadvantage that he stamp is not clobbered by git reset, git clean, etc. Those operations will make stamp invalid but QoLA won't detect it. Storing it in AITER tree where it is seen by git commands alongside to applied patches may be more straightforward
There was a problem hiding this comment.
You make a good point, but even in the worktree it'll be vulnerable to surviving git reset --hard despite the patches vanishing. Instead, maybe we keep it where it is and add a separate invariant/check for determining whether the patches have been correctly applied? I've done so in the latest commit, let me know what you think.
There was a problem hiding this comment.
I think there are few issues with stamp scheme despite where it is stored:
- it does not take into account QoLA itself (for ex. registry) and manifest parameters except aiter commit;
- it may interfere with CK-JIT: either always rebuild because of CK-JIT touches CK codegen, or opposite - skip rebuild on CK-JIT update
As for tree validity detection. Despite desire not to have local AITER/CK modifications it is extremely useful for debugging so there should be a way to forcibly skip AITER source tree preparation but perform build. And it may be OK to ignore local AITER changes and require user to force rebuild in such case. In this case combination of AITER commit + patches hash should be good enough and even detection with 'git apply -R' is not required because any discrepancy is result of 'manual modification'.
So while local modifications are needed source preparation should be suppressed and build either forced or naturally follow files timestamps updates, and returning to clean tree will require one run with force update
| def _patches_applied(aiter_root: str, patches: list[Path]) -> bool: | ||
| """Return True iff every patch in *patches* is present in the working tree. | ||
|
|
||
| ``git apply --reverse --check`` succeeds only when a patch's post-image is |
There was a problem hiding this comment.
I do not think it works this way. Instead it verifies that patch can be applied (reverted) clean. i.e. any non-conflicting modifications are ignored.
| forward path falls back off it — ``--3way`` rejects submodule paths upfront, | ||
| while plain apply reads them from the working tree. | ||
|
|
||
| Costs one ``git apply`` per patch, each reading only the handful of files |
There was a problem hiding this comment.
Comparing to build cost git status or git diff are not expensive. On the other hand git rev-parse HEAD + git diff HEAD should give clean signature of the tree state.
| tracked, patched, or clobbered by ``git reset``/``git submodule update``. | ||
| ``git rev-parse --absolute-git-dir`` resolves both a real ``.git`` | ||
| directory (fresh clone) and a ``.git`` file pointing elsewhere (worktree | ||
| or submodule-style checkout). |
There was a problem hiding this comment.
I think there are few issues with stamp scheme despite where it is stored:
- it does not take into account QoLA itself (for ex. registry) and manifest parameters except aiter commit;
- it may interfere with CK-JIT: either always rebuild because of CK-JIT touches CK codegen, or opposite - skip rebuild on CK-JIT update
As for tree validity detection. Despite desire not to have local AITER/CK modifications it is extremely useful for debugging so there should be a way to forcibly skip AITER source tree preparation but perform build. And it may be OK to ignore local AITER changes and require user to force rebuild in such case. In this case combination of AITER commit + patches hash should be good enough and even detection with 'git apply -R' is not required because any discrepancy is result of 'manual modification'.
So while local modifications are needed source preparation should be suppressed and build either forced or naturally follow files timestamps updates, and returning to clean tree will require one run with force update
2 participants
Motivation
Technical Details
Test Plan
Test Result
Submission Checklist