Use .get method instead of in operator to get value from inference cache safely
#2609
Conversation
β¦e cache safely
|
Although the change makes sense I do worry that you have uncovered a bigger issue with our caches. Have you been able to identify the code that raises the issue? |
Codecov ReportAll modified and coverable lines are covered by tests β
Additional details and impacted files@@ Coverage Diff @@
## main #2609 +/- ##
=======================================
Coverage 93.25% 93.26%
=======================================
Files 93 93
Lines 11068 11069 +1
=======================================
+ Hits 10322 10324 +2
+ Misses 746 745 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Not yet, we run astroid as part of UCX (see links above) which is a tool users can install, run and possibly lint code. I am waiting for the user who detected this issue to help me locate the code causing this bug |
There was a problem hiding this comment.
Thinking about this some more, I wouldn't want to merge it without having seen the code or a reproducer.
I'm worried that there is a good reason to invalidate the cache which causes this KeyError. If that invalidation is correct it would be bad to then return a value here from an already invalidated cache.
|
fyi, I am still waiting to locate the code that creates this error |
|
Thanks, free free to reopen if you can provide a reproducer. Cheers. |
Linked Issues
databrickslabs/ucx#2908 and databrickslabs/ucx#2919
Type of Changes
Description
Use
.getmethod instead ofinoperator to get value from inference cache safelyMotivation
We ran into a
KeyErrorwhen callingNodeNG.inferred(see stack trace below). Unfortunately, I do not have the code that results in theKeyErrorwhen inferring - yet.Stack trace
Theory
I have the following theory, the code fails with a
KeyErroron theyieldhere:Which is odd, as first the
keyis checked to be in the inference cache. Maybe, thekey in context.inferredchecks if thekeyis in the cache, but invalidates the cache at the same time - somehow? There is an_invalidate_cache()in theTransformVisitor._transform()that is in the stack trace and theruntime_checkableis a builtin for which Astroid has a custom lookup that may invalidate the cache.Or, we thread astroid. The thread is probably not thread safe since it is a plain dict, maybe the
KeyErrorwas unlucky timing.Solution
Use the key once on the inference cache by using
.get