Skip to content
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

chore: eliminate calls at import time #5889

Merged
merged 2 commits into from
Dec 2, 2024

Conversation

holmanb
Copy link
Member

@holmanb holmanb commented Nov 19, 2024

It is not best practice, and it often adds unnecessary overhead.

Proposed Commit Message

chore: eliminate calls at import time

It is not best practice, and it often adds unnecessary overhead.

Fixes GH-5344

Additional Context

The important part of this is regex compile - that can be done when/if the function is called rather than at import time which will save some cycles.

While namedtuple() and tuple() aren't truly harmful, there are ways to define similar types statically rather than generating them at runtime, so I modified those in a few places too.

Fixes #5344

It is not best practice, and it often adds unnecessary overhead.
@holmanb holmanb force-pushed the holmanb/do-not-run-code-on-import branch from 19e70eb to 9d39c6d Compare November 19, 2024 23:27
Copy link
Member

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One big inline comment/concern about the regex compiles, but otherwise LGTM.

While namedtuple() and tuple() aren't truly harmful, there are ways to define similar types statically rather than generating them at runtime, so I modified those in a few places too.

Big +1 to changing to new-style named tuples with typing, but is there actually a runtime difference? They both will be evaluated at import time, correct?

@@ -57,7 +55,7 @@ def is_meta_device_name(name):

def is_network_device(name):
# return true if this is a network device
if NETWORK_NAME_RE.match(name):
if re.compile(NETWORK_NAME_FILTER).match(name):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can just be an re.match() because compiling a regex is to pay the time cost once so each subsequent match can be faster.

If the time cost of not being compiled is significant, it may be better to move the compilation into a memoized initialization function. Same comment for the rest of the regexes in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can just be an re.match() because compiling a regex is to pay the time cost once so each subsequent match can be faster.

+1

If the time cost of not being compiled is significant, it may be better to move the compilation into a memoized initialization function. Same comment for the rest of the regexes in this PR.

Every call to re.match() fetches previously-compiled expressions under the covers, so I think this memoization is probably handled for us.

@TheRealFalcon TheRealFalcon self-assigned this Nov 26, 2024
@holmanb
Copy link
Member Author

holmanb commented Nov 27, 2024

Big +1 to changing to new-style named tuples with typing, but is there actually a runtime difference? They both will be evaluated at import time, correct?

Correct - I don't think that there will be much runtime difference - just the benefit of added typing.

@holmanb holmanb requested a review from TheRealFalcon December 2, 2024 21:15
Copy link
Member

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@holmanb holmanb merged commit 3f82153 into canonical:main Dec 2, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strictly disallow import-time code execution
2 participants