-
Notifications
You must be signed in to change notification settings - Fork 53
fix[next]: fix for missing state info in the toolchain cache computations #2104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
5a0a1eb to
a1cf05e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances caching by including workflow-internal state in cache key computations, ensuring cache invalidation when relevant settings change.
- Add a
workflow_state_idproperty toGTFNTranslationStepand other workflow classes. - Update
CachedStepto use a compositecache_key(input hash + workflow state). - Modify tests to use
cache_keyinstead of the old fingerprint function.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/next_tests/.../test_gtfn_module.py | Replaced stages.fingerprint_compilable_program with cache_key. |
| src/gt4py/next/program_processors/codegens/gtfn/gtfn_module.py | Imported utils and added workflow_state_id to GTFNTranslationStep. |
| src/gt4py/next/otf/workflow.py | Introduced workflow_state_id in multiple workflow classes and updated imports. |
Comments suppressed due to low confidence (3)
src/gt4py/next/program_processors/codegens/gtfn/gtfn_module.py:57
- Consider adding a docstring for
workflow_state_idto clarify which internal settings are included in the hash and how it impacts caching behavior.
@functools.cached_property
tests/next_tests/unit_tests/program_processor_tests/codegens_tests/gtfn_tests/test_gtfn_module.py:150
- It would be helpful to add a test verifying that changing the translation step's internal state (e.g.,
device_typeorlanguage_settings) results in a differentcache_key, ensuring cache invalidation works correctly.
cache_key = cached_gtfn_translation_step.cache_key(compilable_program)
src/gt4py/next/otf/workflow.py:174
- The implementation of
workflow_state_idis duplicated across multiple classes (ReplaceEnabledWorkflowMixin,MultiWorkflow,CachedStep,SkippableStep). Consider extracting this into a shared mixin or base class to reduce code duplication.
@functools.cached_property
|
before we forget: by filling the cache with either gtfn_cpu or gtfn_gpu and then switching to the other. With the run for the other backend translation cache created the program for the first backend (can be seen in the hash of the build directory). |
|
Additionally |
Description
Requirements
If this PR contains code authored by new contributors please make sure:
AUTHORS.mdfile adding the names of all the new contributors.