-
Notifications
You must be signed in to change notification settings - Fork 113
write to tmp file & replace, avoid writing a module per process in PointwiseDynamic #532
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
Conversation
…cFunction, add test for multiprocessing & multithreading
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 refactors code caching to use file locks instead of per-process module files and adds tests for concurrent execution of pointwise dynamic functions.
- Introduce
filelock
to guard writes and remove PID suffix from generated filenames inpointwise_dynamic
and various ops. - Update module loading to use stable filenames and drop PID from spec names.
- Add multithreading and multiprocessing tests for
pointwise_dynamic
to ensure correct behavior under concurrency.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
tests/test_pointwise_dynamic.py | Add multithread/multiprocess tests for pointwise dynamic ops |
src/flag_gems/utils/pointwise_dynamic.py | Use filelock around code cache writes; remove PID suffix |
src/flag_gems/ops/tile.py | Add file locking and drop PID in cache filenames |
src/flag_gems/ops/scatter.py | Add file locking and drop PID in cache filenames |
src/flag_gems/ops/repeat.py | Add file locking and drop PID in cache filenames |
src/flag_gems/ops/pad.py | Add file locking and drop PID in cache filenames |
src/flag_gems/ops/index_put.py | Add file locking and drop PID in cache filenames |
src/flag_gems/ops/gather.py | Add file locking and drop PID in cache filenames |
Comments suppressed due to low confidence (1)
src/flag_gems/utils/pointwise_dynamic.py:1263
- Since filenames no longer include a process-unique suffix, the cache may serve stale code when regeneration is needed. Consider incorporating a hash of
code.getvalue()
into the filename or invalidating the cache when the code changes.
file_name = ( f"pointwise_dynamic_{self._scalar_fn_cache_key}_{kernel_name}_... .py")
with filelock.FileLock(lock_path): | ||
if not os.path.exists(file_path): | ||
with open(file_path, "wt", encoding="utf-8") as f: | ||
f.write(code.getvalue()) |
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.
[nitpick] The file-locking and write logic is duplicated across multiple operators. Extract this pattern into a shared utility to reduce duplication and simplify future updates.
with filelock.FileLock(lock_path): | |
if not os.path.exists(file_path): | |
with open(file_path, "wt", encoding="utf-8") as f: | |
f.write(code.getvalue()) | |
write_with_lock(lock_path, file_path, code.getvalue()) |
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
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.
LGTM
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.
LGTM
PR Category
Other
Type of Change
Other
Description
Previously, PointwiseDynamic woule generate module for pointwise functions with the process id as suffix in filename to avoid conflicts in file-writing. It has 2 drawbacks: filenames with pid prevent file reuse between different runs, it generate duplicated files with different names, which is redundant.
Issue
Progress
Performance