-
Notifications
You must be signed in to change notification settings - Fork 38
[Lang] Cut per-compilation Python overhead (sys.modules scan + per-node source formatting) #804
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import inspect | ||
| import linecache | ||
| import re | ||
| import sys | ||
| import typing | ||
|
|
@@ -141,10 +141,13 @@ def pyfunc(fn: Callable) -> QuadrantsCallable: | |
| def _inside_class(level_of_class_stackframe: int) -> bool: | ||
| try: | ||
| maybe_class_frame = sys._getframe(level_of_class_stackframe) | ||
| statement_list = inspect.getframeinfo(maybe_class_frame)[3] | ||
| if statement_list is None: | ||
| # Read the decoration-site source line via linecache rather than inspect.getframeinfo: getframeinfo | ||
| # resolves the frame's module through inspect.getmodule, an O(len(sys.modules)) scan run once per kernel | ||
| # creation. With thousands of modules loaded this dominates kernel build time; linecache.getline returns | ||
| # the same source line with no such scan. | ||
| first_statment = linecache.getline(maybe_class_frame.f_code.co_filename, maybe_class_frame.f_lineno).strip() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a long-lived process redefines kernels from a filename that is already in Useful? React with 👍 / 👎.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. again, this seems like not something we do typically, but silently being incorrect seems not great? |
||
| if not first_statment: | ||
| return False | ||
| first_statment = statement_list[0].strip() | ||
| for pat in _KERNEL_CLASS_STACKFRAME_STMT_RES: | ||
| if pat.match(first_statment): | ||
| return True | ||
|
|
||
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.
When user kernels are defined in modules loaded from zipimport/zipapps/PEX-style archives,
co_filenameis a virtual path like/app.pyz/pkg/mod.py, andlinecache.getline(filename, lineno)returns an empty string unless module globals/loader state are supplied. The previousinspect.getframeinfopath routed throughinspect.findsource()/linecache.getlines(file, module.__dict__), so the loader could provide the source; with this change_inside_classreturnsFalsefor@qd.kernel/@qd.funcmethods in those packaged modules, causing@qd.data_orientednot to wrap class kernels andselfto be treated as a normal kernel argument.Useful? React with 👍 / 👎.
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.
Has this been addressed? If so, how?
I have mixed feelings about this codex comment. On the one hand, we dont use any zipimport style archives. On the other hand, being silently wrong seems not great. We should either explicitly forbid this and throw an exception, or somehow ensure this is correct (eg falling back on the old method), I feel.