Conversation
We add a hashing to the AST directly to check if the workunit needs to be recompiled. NOTE: Ideally, we could edit the functor.hpp directly so we do not need to bring in all of Kokkos again, but this approach works for now.
|
|
||
| # get parser signature | ||
| signature = ast.dump(self.tree) | ||
| self.signature = hashlib.md5(signature.encode()).hexdigest() |
There was a problem hiding this comment.
how expensive is this? I assume our ast's for kernels are small that this should not be an issue?
There was a problem hiding this comment.
I didn't do a performance test. This block is only done when the workunit is first seen by pykokkos, so it is a startup cost that will not add overhead during repeated execution.
In most cases the ast's should be relatively small; e.g., in my Ewald code, the slowest time for this block of code is 0.013 seconds.
| except AssertionError as e: | ||
| raise AssertionError( | ||
| f"kernel is incorrect\nactual: {arr_correct}\ndesired: {expected}" | ||
| ) from e |
There was a problem hiding this comment.
this test does not check that values of signature on disk changed or something was renamed
There was a problem hiding this comment.
I kept it abstract in case we want to change in implementation details down the road (i.e., if we want to move away from renaming or change the directory structure).
gliga
left a comment
gliga
left a comment
There was a problem hiding this comment.
Overall, good change. See comments.
|
I do not like the renaming part, but we can keep it and see when/if it breaks. |
|
Are we ever going to change the name of that directory? It has been a while that we wanted to move to |
Changed to |
| if ast_signature is not None: | ||
| out_dir = out_dir / f"AST_{ast_signature}" |
There was a problem hiding this comment.
hmm so we have a hierarchy which has the ast signature as the highest priority ... but is there cases where the ast signature would not trigger but the others would?
There was a problem hiding this comment.
Yes, consider the scenario:
- workunit v0 is compiled with pk.float arrays;
- an edit is made and we now have ATS has for workunit v1;
- workunit v1 is compiled with pk.double arrays;
- workunit v1 is called again with pk.float arrays. In this case, the type hash would trigger, but the ats hash would not as it is out of date.
There was a problem hiding this comment.
but doesn't the AST change if we change types from float to double? Even unmanaged Views change their type, I am surprised that this would not trigger an AST hash fail
There was a problem hiding this comment.
Yes, the AST will change, but we want to keep both float and double types cached in this scenario to avoid excessive recompilation (e.g., an adaptive time-stepper may use a float and double execution to get error estimates)
There was a problem hiding this comment.
I see. Could you add a comment explaining this intent? Future selves will thank us for it
* Change ordering of comments for clarity; * remove vestigial try/except blocks
3 participants
Closes #100, #234.
Features
1. AST-hashed compilation directories
pk_cppoutput directories now include anAST_<hash>subdirectory that encodes a structural hash of the compiled workunit. On each invocation, PyKokkos computes the current AST hash and compares it against the on-disk directory name; if the AST hashes differ, recompilation is triggered. This replaces the previous approach which did not check if the compiled AST matched the current AST.2. Incremental recompilation on AST change
When a workunit's AST hash changes,
initialize_directoryrenames the oldAST_<hash>directory to the new hash rather than discarding it. OnlyCMakeCache.txtandcmake.check_cacheare deleted (as they contain hardcoded absolute paths that CMake rejects after a rename), while all object files and link artifacts inbuild/are preserved. This allows CMake to perform an incremental rebuild rather than compiling from scratch.3. Recompilation unit test
Adds
test_recompilationwhich verifies end-to-end that modifying a@pk.workunitbody triggers a correct recompile. The test writes a buggy kernel (arr[i] += 2), runs it and asserts the output, then overwrites it with the fixed kernel (arr[i] += 1) and asserts the recompiled output, confirming that stale builds are invalidated and that the new kernel produces correct results.