Reintroduce dependency optimization #626

trottomv · 2023-05-31T06:44:47Z

woodruffw · 2023-05-31T17:36:21Z

Thanks for opening this! I'll have time to do a review in the coming days.

woodruffw

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.

woodruffw · 2023-06-03T01:37:26Z

pip_audit/_cli.py

+    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",
+    )


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.

woodruffw · 2023-06-03T01:38:03Z

pip_audit/_dependency_source/requirement.py

@@ -44,6 +53,7 @@ def __init__(
        index_url: str | None = None,
        extra_index_urls: list[str] = [],
        state: AuditState = AuditState(),
+        with_pip_compile: bool = False,


Same comment as above: I don't think we want to imply that this is unique to pip-compile, since it wasn't before.

woodruffw · 2023-06-03T01:39:34Z

pip_audit/_dependency_source/requirement.py

+        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))


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.

trottomv · 2023-06-05T16:58:15Z

@woodruffw I've removed the "with_pip_compile" option and restored the preresolved dependencies collection as in #540

woodruffw · 2023-06-05T17:17:11Z

Cool, thank you! I'll have time to review again in the coming days.

pauloxnet · 2023-06-06T06:22:09Z

It seems ok to me. Maybe you can update the Changelog file.

woodruffw · 2023-06-07T16:52:03Z

Lint failure:

would reformat pip_audit/_dependency_source/interface.py
would reformat pip_audit/_dependency_source/requirement.py
would reformat test/dependency_source/test_requirement.py

Oh no! 💥 💔 💥
3 files would be reformatted, 48 files would be left u

You can use make reformat and make lint locally to catch these.

…introduce-dependecy-optimization

pauloxnet · 2023-06-08T08:46:29Z

LGTM

CHANGELOG.md

woodruffw

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.

tetsuo-cpp · 2023-06-17T03:50:30Z

Sorry for the delay @trottomv! I'll get to this tonight.

tetsuo-cpp

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.

tetsuo-cpp · 2023-06-19T02:11:11Z

pip_audit/_dependency_source/requirement.py

+        # 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:


@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.

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.

@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 🙂

No problem to me, if I can contribute, I'm happy to do so, time permitting.

@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.

resolution path

tetsuo-cpp · 2023-06-22T09:15:18Z

@woodruffw, I've made those changes now. Could you take a quick look at this?

pip_audit/_cli.py

woodruffw

LGTM!

woodruffw · 2023-06-28T16:45:46Z

Thanks for all of the hard work here @trottomv! Merging.

woodruffw self-requested a review May 31, 2023 17:36

woodruffw added the component:dep-sources Dependency sources label May 31, 2023

woodruffw requested changes Jun 3, 2023

View reviewed changes

trottomv force-pushed the trottomv/reintroduce-dependecy-optimization branch 5 times, most recently from 21fe2e7 to 97b92cf Compare June 5, 2023 16:55

trottomv force-pushed the trottomv/reintroduce-dependecy-optimization branch 2 times, most recently from c6a7693 to aeed974 Compare June 6, 2023 06:35

Reintroduce preresolved dependencies optimization

9674a34

trottomv force-pushed the trottomv/reintroduce-dependecy-optimization branch from aeed974 to 9674a34 Compare June 6, 2023 06:38

woodruffw assigned trottomv Jun 7, 2023

trottomv added 3 commits June 8, 2023 08:31

Fix lint

9afdfb3

Merge branch 'main' of github.com:trottomv/pip-audit into trottomv/re…

a6bf10e

…introduce-dependecy-optimization

Merge branch 'main' of github.com:trottomv/pip-audit into trottomv/re…

c744f49

…introduce-dependecy-optimization

pauloxnet approved these changes Jun 8, 2023

View reviewed changes

pauloxnet reviewed Jun 8, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

trottomv force-pushed the trottomv/reintroduce-dependecy-optimization branch from 6e0a42a to 8be732a Compare June 8, 2023 12:37

Fix CHANGELOG

783779d

trottomv force-pushed the trottomv/reintroduce-dependecy-optimization branch from 8be732a to 783779d Compare June 8, 2023 12:44

trottomv and others added 2 commits June 9, 2023 07:32

Merge branch 'main' into trottomv/reintroduce-dependecy-optimization

8eff6d5

Merge branch 'main' into trottomv/reintroduce-dependecy-optimization

66f5151

woodruffw approved these changes Jun 9, 2023

View reviewed changes

Merge branch 'main' into trottomv/reintroduce-dependecy-optimization

8ff8a85

woodruffw requested a review from tetsuo-cpp June 13, 2023 17:08

Merge branch 'main' into trottomv/reintroduce-dependecy-optimization

f0a1c43

tetsuo-cpp approved these changes Jun 19, 2023

View reviewed changes

Merge branch 'main' into trottomv/reintroduce-dependecy-optimization

6175bfd

tetsuo-cpp self-assigned this Jun 22, 2023

tetsuo-cpp added 6 commits June 22, 2023 17:07

_cli, requirement: Introduce --disable-pip flag

59b68f1

Merge branch 'main' into trottomv/reintroduce-dependecy-optimization

abaf0e3

README: Update help text

98626d0

requirement: Add back the --require-hashes flag in the dependency

7d932ae

resolution path

test: Fix tests and fill in test coverage

bd34c41

CHANGELOG: Add more detail to changelog

632085c

tetsuo-cpp requested a review from woodruffw June 28, 2023 01:49

Merge branch 'main' into trottomv/reintroduce-dependecy-optimization

87c738c

woodruffw reviewed Jun 28, 2023

View reviewed changes

pip_audit/_cli.py Outdated Show resolved Hide resolved

Update pip_audit/_cli.py

fa0b146

woodruffw approved these changes Jun 28, 2023

View reviewed changes

woodruffw enabled auto-merge (squash) June 28, 2023 16:45

woodruffw merged commit d93c5c8 into pypa:main Jun 28, 2023
7 checks passed

woodruffw mentioned this pull request Jul 2, 2023

Reintroduce the preresolved dependency optimization #610

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reintroduce dependency optimization #626

Reintroduce dependency optimization #626

trottomv commented May 31, 2023 •

edited

Loading

woodruffw commented May 31, 2023

woodruffw left a comment

woodruffw Jun 3, 2023

woodruffw Jun 3, 2023

woodruffw Jun 3, 2023

trottomv commented Jun 5, 2023

woodruffw commented Jun 5, 2023

pauloxnet commented Jun 6, 2023

woodruffw commented Jun 7, 2023

pauloxnet commented Jun 8, 2023

woodruffw left a comment

tetsuo-cpp commented Jun 17, 2023

tetsuo-cpp left a comment

tetsuo-cpp Jun 19, 2023

trottomv Jun 19, 2023 •

edited

Loading

woodruffw Jun 20, 2023

trottomv Jun 20, 2023 •

edited

Loading

tetsuo-cpp Jun 22, 2023

tetsuo-cpp commented Jun 22, 2023

woodruffw left a comment

woodruffw commented Jun 28, 2023

Reintroduce dependency optimization #626

Reintroduce dependency optimization #626

Conversation

trottomv commented May 31, 2023 • edited Loading

woodruffw commented May 31, 2023

woodruffw left a comment

Choose a reason for hiding this comment

woodruffw Jun 3, 2023

Choose a reason for hiding this comment

woodruffw Jun 3, 2023

Choose a reason for hiding this comment

woodruffw Jun 3, 2023

Choose a reason for hiding this comment

trottomv commented Jun 5, 2023

woodruffw commented Jun 5, 2023

pauloxnet commented Jun 6, 2023

woodruffw commented Jun 7, 2023

pauloxnet commented Jun 8, 2023

woodruffw left a comment

Choose a reason for hiding this comment

tetsuo-cpp commented Jun 17, 2023

tetsuo-cpp left a comment

Choose a reason for hiding this comment

tetsuo-cpp Jun 19, 2023

Choose a reason for hiding this comment

trottomv Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

woodruffw Jun 20, 2023

Choose a reason for hiding this comment

trottomv Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

tetsuo-cpp Jun 22, 2023

Choose a reason for hiding this comment

tetsuo-cpp commented Jun 22, 2023

woodruffw left a comment

Choose a reason for hiding this comment

woodruffw commented Jun 28, 2023

trottomv commented May 31, 2023 •

edited

Loading

trottomv Jun 19, 2023 •

edited

Loading

trottomv Jun 20, 2023 •

edited

Loading