-
Notifications
You must be signed in to change notification settings - Fork 455
chore(core): rename and document core APIs for clarity #14416
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
|
f2b0b30
to
f13a97b
Compare
Bootstrap import analysisComparison of import times between this PR and base. SummaryThe average import time from this PR is: 270 ± 5 ms. The average import time from base is: 271 ± 4 ms. The import time difference between this PR and base is: -0.3 ± 0.2 ms. The difference is not statistically significant (z = -1.71). Import time breakdownThe following import paths have shrunk:
|
View all feedbacks in Devflow UI.
This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals.
[email protected] unqueued this merge request |
/remove |
View all feedbacks in Devflow UI.
|
Performance SLOsCandidate: brettlangdon/rename.core.get_item (20c74cf) 🔵 No Baseline Data (24 suites)🔵 coreapiscenario - 12/12 (2 unstable)🔵 No baseline data available for this suite
|
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 for DSM
#14404 helped show that people should be clear/intentional over which core API is used for getting items off of an
ExecutionContext
.This PR aims to rename and update doc strings to try and clarify further what will happen when the function is called.
get_item
->find_item
: since this API will traverse the context tree looking for the first value for the key. normally this adds the overhead of a loop instruction, but in general will return back quickly. The problem comes from cache misses where we traverse the whole tree just to return the default value.get_local_item
->get_item
: make it so any new usage ofget_item
does the more correct thing by defaultget_items
->find_items
: similar asfind_item
, making it more clear we iterate the whole context tree for each keyMain open question still is, what should
__getitem__
do? this one is harder to refactor without knowing 100% of all usages ofctx["key"]
in the codebase, so we should handle as a follow-up since it might have higher impact. Opinion: we should remove the__getitem__
API and force people to useget_item
orfind_item
for clarity.. the overhead should be the same since they are all Python function calls.Checklist
Reviewer Checklist