-
Notifications
You must be signed in to change notification settings - Fork 64
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
Reintroduce dependency optimization #626
Reintroduce dependency optimization #626
Conversation
Thanks for opening this! I'll have time to do a review in the coming days. |
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 again for opening this!
This is a good start, but it's going to need a few changes before I think we can consider merging this.
In particular, my recommendation is to go back and look at the optimization's state at the time it was removed in #540 -- the re-addition of the optimization here should closely mirror that code, rather than being an independent reimplementation. That will save us a lot of heartburn and trouble down the road.
pip_audit/_cli.py
Outdated
parser.add_argument( | ||
"--with-pip-compile", | ||
action="store_true", | ||
help="don't perform dependency installation in a temporary virtual env" | ||
" if requirements files is builded with pip compile; requires --requirement argument", | ||
) |
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 implies that hashed requirements files always come from pip-compile
, which isn't true.
Rather than adding a new flag here, let's revert the behavior that was removed with #540: the pre-existing --no-deps
and --require-hashes
flags should cause us to go down an optimized path.
@@ -44,6 +53,7 @@ def __init__( | |||
index_url: str | None = None, | |||
extra_index_urls: list[str] = [], | |||
state: AuditState = AuditState(), | |||
with_pip_compile: bool = False, |
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.
Same comment as above: I don't think we want to imply that this is unique to pip-compile
, since it wasn't before.
parsed_packages = [] | ||
for filename in filenames: | ||
requirements_file = RequirementsFile.from_file(filename) | ||
requirements_dict = requirements_file.to_dict()["requirements"] | ||
parsed_packages += [ | ||
(r["name"], r["specifier"][0].split("==")[1]) for r in requirements_dict | ||
] | ||
for name, version in parsed_packages: | ||
yield ResolvedDependency(name=name, version=Version(version)) |
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 needs to be hardened against URL-style requirements, parser errors, etc.
My suggestion would be to look at what was removed in #540, and reuse that to the greatest extent possible: it had a lot of "scar tissue" from different bugs reported over time.
21fe2e7
to
97b92cf
Compare
@woodruffw I've removed the "with_pip_compile" option and restored the preresolved dependencies collection as in #540 |
Cool, thank you! I'll have time to review again in the coming days. |
It seems ok to me. Maybe you can update the Changelog file. |
c6a7693
to
aeed974
Compare
aeed974
to
9674a34
Compare
Lint failure:
You can use |
…introduce-dependecy-optimization
…introduce-dependecy-optimization
LGTM |
6e0a42a
to
8be732a
Compare
8be732a
to
783779d
Compare
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; I'll do some end-CLI testing this evening.
I'd like @tetsuo-cpp to also give this a review, since he wrote the original fast-path here.
Sorry for the delay @trottomv! I'll get to this tonight. |
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.
Code looks good!
@woodruffw Regarding my comment about the flag, I don't feel too strongly about it either way, it just wasn't what I was expecting. If that seems good to you, feel free to merge.
# If the user has supplied `--no-deps` or there are hashed requirements, we should assume | ||
# that we have a fully resolved set of dependencies and we should waste time by invoking | ||
# `pip`. | ||
if self._no_deps or require_hashes: |
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.
@woodruffw I thought the plan was that we were going to give it some kind of scary name instead of having it activate as soon as we supply a hashed requirement since the fast path is less comprehensive than doing a real pip
installation.
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.
Hi @tetsuo-cpp thanks a lot for your review.
I agree with you. In my humble opinion, invoking "preresolved dependency" based solely on the requirement presenting the hashes is a bit weak. My original idea was to add a new flag, and I explained it better in this comment on the issue. #610 (comment)
I see also your approval, feel free to merge, We can potentially continue the discussion to evaluate different solutions if necessary.
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.
@tetsuo-cpp You're right, I forgot about that -- IMO --disable-pip
or similar would be appropriate.
The semantics there would be:
--disable-pip
will cause an error unless we're in a requirements input mode--disable-pip
will cause an error unless the requirements are fully hashed or the user also passes--no-deps
Does that sound reasonable to you?
@trottomv I'd prefer to merge this as a single feature, so we'll probably want to settle the new flag before merging 🙂
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.
No problem to me, if I can contribute, I'm happy to do so, time permitting.
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.
@woodruffw Yep! Those flag names sound good to me.
@trottomv Sorry for the churn! I'm planning to push those change to your branch this afternoon and we can look at getting this released.
@woodruffw, I've made those changes now. Could you take a quick look at this? |
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!
Thanks for all of the hard work here @trottomv! Merging. |
Fix #610