-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Enable caching functions arising from exec of strings. #6406
Enable caching functions arising from exec of strings. #6406
Conversation
This needs more thought about uniquely capturing a
The file path is also a bit strange and needs looking at. It also needs tests. |
First test for string source functions
93eaa15
to
e0e0f14
Compare
for x in py_func.__code__.co_consts: | ||
buf.append(str(x)) | ||
const_bytes = (''.join(buf)).encode('UTF-8') |
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.
Why not just do a repr(co_consts).encode()
?
str
on individual elements makes it the following looks the same:
(1, 1)
(11,)
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.
WIP :), will fix, thanks!
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.
From a vague memory, I think I put this in when messing about locally as it was an easy thing to trigger as part of the identifier.
for x in py_func.__code__.co_consts: | ||
buf.append(str(x)) | ||
const_bytes = (''.join(buf)).encode('UTF-8') | ||
data = py_func.__code__.co_code + const_bytes |
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.
This won't pick up changes in function name, function arguments, etc. Maybe consider all states in https://github.com/python/cpython/blob/66d3b589c44fcbcf9afe1e442d9beac3bd8bcd34/Objects/codeobject.c#L989-L1012
Might also need the function type signature as this does not store the value of default args.
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.
thanks, I've been considering #6406 (comment) and generally poking at the contents of codeobject
to see what can/cannot be easily worked out.
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.
My main concern here is freevars and globals, and wiring through enough data to achieve accurate and correct behaviour.
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.
I am just not sure if we can handle freevars and globals as is. I am open to the idea of making cacheable functions handle freevars and globals differently than non-cacheable functions. I'm thinking making them linked at load time.
Wow, I feel this will be a nice feature. SDC also uses many string-format jit function via Any updates on this PR? |
Resolves conflicts in: numba/core/caching.py numba/tests/test_dispatcher.py
As title.
f = mod.str_closure1 | ||
self.assertPreciseEqual(f(3), 6) # 3 + 3 = 6 | ||
f = mod.str_closure2 | ||
self.assertPreciseEqual(f(3), 8) # 3 + 5 = 8 | ||
f = mod.str_closure3 | ||
self.assertPreciseEqual(f(3), 10) # 3 + 7 = 8 | ||
f = mod.str_closure4 | ||
self.assertPreciseEqual(f(3), 12) # 3 + 9 = 12 | ||
self.check_pycache(5) # 1 nbi, 4 nbc |
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.
Normally when the environment is rebuilt as part of cache replay the module for the function is loaded so as to provide globals etc. These tests are failing because there's no module associated with a str exec function and the function refers to a variable that is in the globals supplied to exec.
I think most of the test failures in the above are because the cache file presence check is looking in a |
This pull request is marked as stale as it has had no activity in the past 3 months. Please respond to this comment if you're still interested in working on this. Many thanks! |
This pull request is marked as stale as it has had no activity in the past 3 months. Please respond to this comment if you're still interested in working on this. Many thanks! |
Reopening as there's still interest in this work. |
This pull request is marked as stale as it has had no activity in the past 3 months. Please respond to this comment if you're still interested in working on this. Many thanks! |
I would like to catch up with this abandoned PR. If I can find a workable solution based on it, will open a PR to continue. I feel this draft has already worked? Just need to merge with main, and fix some corner-cases? |
As title.