-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Python: Allow absolute imports from source directory #5614
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
Python: Allow absolute imports from source directory #5614
Conversation
This greatly restricts the set of modules that have a new name under this scheme. One change to the tests was needed, which reflects the fact that the two `main.py` files no longer have the name `main` (which makes sense, since they're never imported under this name).
…ttps://github.com/tausbn/codeql into python-allow-absolute-imports-from-source-directory
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 would consider renaming maybeExecutedDirectly
to isPossibleEntryPoint
to unify the abstractions. I think it would make mayContainEntryPoint
and transitively_imported_from_entry_point
read as (even more) obvious.
Is an evaluation running? |
Ah, I'll also change |
Evaluation is running here: https://github.com/dsp-testing/tausbn-dca/tree/run/python-allow-absolute-imports-3/reports |
Evaluation is done. No noticeable change in performance. It's possible that there's a slight slowdown (as this PR permits more analysis to be done), but I think it's so small it's lost in the noise. |
Is this not supposed to be a statistically significant 3% slowdown? It might still be small enough that we allow it, but I thought the idea was that we could trust these numbers to not be lost in the noise? |
¯\_(ツ)_/¯ I mean, the slowdown of (say) ~4% that corresponds to an 8 second increase in time may be statistically significant, but I wouldn't personally trust it off the bat. The larger projects seem more reliable in this regard (where, indeed, |
I ran a further performance test, using the entire Code Scanning suite. Results seem to indicate a 4% slowdown overall. I think this is probably acceptable in the short run. |
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.
Indeed, I am happy to trade a small performance penalty for less surprising and frustrating behaviour.
Do we know of any projects that might be pushed over the edge but this (so that we could attempt some targeted optimisations up-front rather than last-minute)?
Alright, in it goes. |
Limits the behaviour of github#5614 in two ways: First, we only consider files that are contained in the source archive. This prevents unnecessary computation involving files in e.g. the standard library. Secondly, we ignore any relative imports (e.g. `from .foo import ...`), as these only work inside packages anyway. This fixes an observed performance regression on projects that include `google-cloud-sdk` as part of their source code.
#5506 revisited, and #5552 reverted.
To fix the performance issues, we greatly restrict the set of modules that have a new name under this scheme. The previous PR considered any file that coexisted with an executable Python script to be importable using its name as an absolute import. This PR limits itself to just those files that are actually imported.
One change to the tests was needed, which reflects the fact that the two
main.py
files no longer have the namemain
(which makes sense, since they're never imported under this name). Crucially,module
is still a valid name for both of themodule.py
files