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

Make dry-run script twice as fast #5950

Merged
merged 1 commit into from
Oct 24, 2022
Merged

Conversation

deivid-rodriguez
Copy link
Contributor

Check whether up to date after figuring latest version.

Before, on a standard Python update:

$ time bin/dry-run.rb pip LeeeeT/valtypes --cache files
To use retry middleware with Faraday v2.0+, install `faraday-retry` gem
=> reading dependency files from cache manifest: ./dry-run/LeeeeT/valtypes/cache-manifest-pip.json
=> parsing dependency files
=> updating 11 dependencies: pre-commit, isort, sort-all, black, pyproject-flake8, mypy, pyright, pytest, pytest-cov, mkdocs-material, jinja2

=== pre-commit ()
 => checking for updates 1/11
🌍 https://pypi.org/simple/pre-commit/
 => latest available version is 2.20.0
 => latest allowed version is 2.20.0
    (no update needed as it's already up-to-date)

(...)

=== jinja2 (3.1.0)
 => checking for updates 11/11
🌍 https://pypi.org/simple/jinja2/
 => latest available version is 3.1.2
🌍 https://pypi.org/simple/jinja2/
 => latest allowed version is 3.1.2
 => requirements to unlock: own
 => requirements update strategy: bump_versions
 => updating jinja2 from 3.1.0 to 3.1.2

    ± docs/requirements.txt
    ~~~
    2c2
    < Jinja2==3.1.0
    ---
    > Jinja2==3.1.2
    ~~~
🌍 Total requests made: '24'

real	1m0.863s
user	0m35.599s
sys	0m7.230s

After this change:

$ time bin/dry-run.rb pip LeeeeT/valtypes --cache files
To use retry middleware with Faraday v2.0+, install `faraday-retry` gem
=> reading dependency files from cache manifest: ./dry-run/LeeeeT/valtypes/cache-manifest-pip.json
=> parsing dependency files
=> updating 11 dependencies: pre-commit, isort, sort-all, black, pyproject-flake8, mypy, pyright, pytest, pytest-cov, mkdocs-material, jinja2

=== pre-commit ()
 => checking for updates 1/11
🌍 https://pypi.org/simple/pre-commit/
 => latest available version is 2.20.0
    (no update needed as it's already up-to-date)

(...)

=== jinja2 (3.1.0)
 => checking for updates 11/11
🌍 https://pypi.org/simple/jinja2/
 => latest available version is 3.1.2
🌍 https://pypi.org/simple/jinja2/
 => latest allowed version is 3.1.2
 => requirements to unlock: own
 => requirements update strategy: bump_versions
 => updating jinja2 from 3.1.0 to 3.1.2

    ± docs/requirements.txt
    ~~~
    2c2
    < Jinja2==3.1.0
    ---
    > Jinja2==3.1.2
    ~~~
🌍 Total requests made: '24'

real	0m27.956s
user	0m9.490s
sys	0m3.146s

So, more than 50% speed up.

And I think this also matches more closely what the real updater does.

@deivid-rodriguez deivid-rodriguez requested a review from a team as a code owner October 22, 2022 09:08
@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/faster-dry-run branch from 7df7a89 to 5a099b1 Compare October 22, 2022 09:11
@deivid-rodriguez
Copy link
Contributor Author

This is @jerbob92's idea. I'm just committing what he suggested, hence the co-authorship of this PR.

Copy link
Member

@jurre jurre left a comment

Choose a reason for hiding this comment

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

Not sure if this is problematic, as the speedup is nice, but I think this does introduce a small functional change, for a security update, when a dependency is already up-to-date but also not vulnerable we no longer print the (no security update needed as it's not vulnerable) message anymore (or the error message in the chase the version class isn't correct).

Does this align with what we do in production? Should we also make this change there?

@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/faster-dry-run branch from 5a099b1 to 749c9a5 Compare October 22, 2022 17:00
@deivid-rodriguez
Copy link
Contributor Author

deivid-rodriguez commented Oct 22, 2022

Nice catch @jurre, I had moved the check too early. I moved it after the message you mentioned now and we still get similar speed up. Have a look now!

Regarding production, I did have a quick look at the code and I did not see the possibility to make this same speed up. I think we're good there.

@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/faster-dry-run branch from 749c9a5 to 82357dd Compare October 24, 2022 07:38
Before, on a standard Python update:

```
$ time bin/dry-run.rb pip LeeeeT/valtypes --cache files
To use retry middleware with Faraday v2.0+, install `faraday-retry` gem
=> reading dependency files from cache manifest: ./dry-run/LeeeeT/valtypes/cache-manifest-pip.json
=> parsing dependency files
=> updating 11 dependencies: pre-commit, isort, sort-all, black, pyproject-flake8, mypy, pyright, pytest, pytest-cov, mkdocs-material, jinja2

=== pre-commit ()
 => checking for updates 1/11
🌍 https://pypi.org/simple/pre-commit/
 => latest available version is 2.20.0
 => latest allowed version is 2.20.0
    (no update needed as it's already up-to-date)

(...)

=== jinja2 (3.1.0)
 => checking for updates 11/11
🌍 https://pypi.org/simple/jinja2/
 => latest available version is 3.1.2
🌍 https://pypi.org/simple/jinja2/
 => latest allowed version is 3.1.2
 => requirements to unlock: own
 => requirements update strategy: bump_versions
 => updating jinja2 from 3.1.0 to 3.1.2

    ± docs/requirements.txt
    ~~~
    2c2
    < Jinja2==3.1.0
    ---
    > Jinja2==3.1.2
    ~~~
🌍 Total requests made: '24'

real	1m0.863s
user	0m35.599s
sys	0m7.230s
```

After this change:

```
$ time bin/dry-run.rb pip LeeeeT/valtypes --cache files
To use retry middleware with Faraday v2.0+, install `faraday-retry` gem
=> reading dependency files from cache manifest: ./dry-run/LeeeeT/valtypes/cache-manifest-pip.json
=> parsing dependency files
=> updating 11 dependencies: pre-commit, isort, sort-all, black, pyproject-flake8, mypy, pyright, pytest, pytest-cov, mkdocs-material, jinja2

=== pre-commit ()
 => checking for updates 1/11
🌍 https://pypi.org/simple/pre-commit/
 => latest available version is 2.20.0
    (no update needed as it's already up-to-date)

(...)

=== jinja2 (3.1.0)
 => checking for updates 11/11
🌍 https://pypi.org/simple/jinja2/
 => latest available version is 3.1.2
🌍 https://pypi.org/simple/jinja2/
 => latest allowed version is 3.1.2
 => requirements to unlock: own
 => requirements update strategy: bump_versions
 => updating jinja2 from 3.1.0 to 3.1.2

    ± docs/requirements.txt
    ~~~
    2c2
    < Jinja2==3.1.0
    ---
    > Jinja2==3.1.2
    ~~~
🌍 Total requests made: '24'

real	0m27.956s
user	0m9.490s
sys	0m3.146s
```

So, more than 50% speed up.

And I think this also matches more closely what the real updater does.

Co-authored-by: Jeroen Bobbeldijk <[email protected]>
@jeffwidman jeffwidman force-pushed the deivid-rodriguez/faster-dry-run branch from 82357dd to 94db1e1 Compare October 24, 2022 07:47
@jeffwidman jeffwidman enabled auto-merge October 24, 2022 07:49
@jeffwidman jeffwidman merged commit 1b1f0ae into main Oct 24, 2022
@jeffwidman jeffwidman deleted the deivid-rodriguez/faster-dry-run branch October 24, 2022 07:58
@pavera pavera mentioned this pull request Oct 31, 2022
@jeffwidman
Copy link
Member

I think this does introduce a small functional change, for a security update, when a dependency is already up-to-date but also not vulnerable we no longer print the (no security update needed as it's not vulnerable) message anymore (or the error message in the chase the version class isn't correct).

I ran into a variant of this debugging the pub security updates where the dep is up to date and vulnerable, so slightly tweaked the order here:

jeffwidman added a commit that referenced this pull request Dec 5, 2022
In #5950, this
short-circuit was moved up to skip checking for the latest version if
we already know we're on the latest version.

During review, it was moved below the case for a security update, when a
dep is already up-to-date but also _not_ vulnerable.

However, there's yet another case, which is where a dep is up-to-date
and _also_ vulnerable. I was surprised in
#6230 (comment)
to see that it spits out `no update needed as it's already up-to-date`
when I had explicitly passed in a security advisor marking the latest
version as vulnerable. It wasn't clear to me if the `dry-run` was
prematurely bailing before checking for vulnerable versions. That was
for the `pub` ecosystem, where both
`checker.lowest_resolvable_security_fix_version` and
`checker.latest_resolvable_version` return the same version.

The very next `if` block handles the case when the dep _is_ vulnerable.
So by moving this short-circuit below that, it will still short circuit
for the common case where a version is not vulnerable and on latest.

For reference, here's pre-change run:
```
[dependabot-core-dev] ~/dependabot-core $ SECURITY_ADVISORIES='[{"dependency-name":"buffer","patched-versions":[],"unaffected-versions":[],"affected-versions":["<= 2.0.0"]}]' bin/dry-run.rb pub dart-lang/pub-dev --dir "/app" --cache=files --dep="buffer" # direct dep
warning: parser/current is loading parser/ruby31, which recognizes3.1.3-compliant syntax, but you are running 3.1.2.
Please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
=> reading cloned repo from /home/dependabot/dependabot-core/tmp/dart-lang/pub-dev
=> parsing dependency files
I, [2022-12-04T22:26:35.101963 #38484]  INFO -- : Failed to infer the flutter version. Attempting to use latest stable release.
I, [2022-12-04T22:26:35.102107 #38484]  INFO -- : Cloning the flutter repo https://github.com/flutter/flutter.
I, [2022-12-04T22:27:40.833227 #38484]  INFO -- : Checking out Flutter version stable
I, [2022-12-04T22:27:46.471869 #38484]  INFO -- : Running `flutter doctor` to install artifacts and create flutter/version.
I, [2022-12-04T22:29:39.504293 #38484]  INFO -- : Running `flutter --version`
I, [2022-12-04T22:29:40.773354 #38484]  INFO -- : Installed the Flutter SDK version: 3.3.9 with Dart 2.18.5.
=> updating 1 dependencies: buffer

=== buffer (1.1.1) (vulnerable 🚨)
 => checking for updates 1/1
I, [2022-12-04T22:29:41.511587 #38484]  INFO -- : Failed to infer the flutter version. Attempting to use latest stable release.
I, [2022-12-04T22:29:41.511704 #38484]  INFO -- : Checking out Flutter version stable
I, [2022-12-04T22:29:42.735602 #38484]  INFO -- : Running `flutter doctor` to install artifacts and create flutter/version.
I, [2022-12-04T22:29:45.652134 #38484]  INFO -- : Running `flutter --version`
I, [2022-12-04T22:29:46.743673 #38484]  INFO -- : Installed the Flutter SDK version: 3.3.9 with Dart 2.18.5.
 => latest available version is 1.1.1
    (no update needed as it's already up-to-date)
🌍 Total requests made: '0'
```

And here's post-change run:
```
[dependabot-core-dev] ~/dependabot-core $ SECURITY_ADVISORIES='[{"dependency-name":"buffer","patched-versions":[],"unaffected-versions":[],"affected-versions":["<= 2.0.0"]}]' bin/dry-run.rb pub dart-lang/pub-dev --dir "/app" --cache=files --dep="buffer" # direct dep
warning: parser/current is loading parser/ruby31, which recognizes3.1.3-compliant syntax, but you are running 3.1.2.
Please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
=> reading cloned repo from /home/dependabot/dependabot-core/tmp/dart-lang/pub-dev
=> parsing dependency files
I, [2022-12-04T22:44:40.544797 #39357]  INFO -- : Failed to infer the flutter version. Attempting to use latest stable release.
I, [2022-12-04T22:44:40.544922 #39357]  INFO -- : Checking out Flutter version stable
I, [2022-12-04T22:44:41.954572 #39357]  INFO -- : Running `flutter doctor` to install artifacts and create flutter/version.
I, [2022-12-04T22:44:45.209362 #39357]  INFO -- : Running `flutter --version`
I, [2022-12-04T22:44:46.159503 #39357]  INFO -- : Installed the Flutter SDK version: 3.3.9 with Dart 2.18.5.
=> updating 1 dependencies: buffer

=== buffer (1.1.1) (vulnerable 🚨)
 => checking for updates 1/1
I, [2022-12-04T22:44:46.554930 #39357]  INFO -- : Failed to infer the flutter version. Attempting to use latest stable release.
I, [2022-12-04T22:44:46.555046 #39357]  INFO -- : Checking out Flutter version stable
I, [2022-12-04T22:44:47.709661 #39357]  INFO -- : Running `flutter doctor` to install artifacts and create flutter/version.
I, [2022-12-04T22:44:51.640507 #39357]  INFO -- : Running `flutter --version`
I, [2022-12-04T22:44:52.616051 #39357]  INFO -- : Installed the Flutter SDK version: 3.3.9 with Dart 2.18.5.
 => latest available version is 1.1.1
 => there is no available non-vulnerable version
    (no update needed as it's already up-to-date)
🌍 Total requests made: '0'
```

Note that the output above is slightly confusing because it's _not_
doing a security update only, but rather a normal update with a
specified security advisory... so I think it's fine for debugging
purposes that we get the information that there's no non-vulnerable
version, and also that from a version-checking perspective there's no
available update, regardless of vulnerability.
jeffwidman added a commit that referenced this pull request Dec 5, 2022
In #5950, this
short-circuit was moved up to skip checking for the latest version if
we already know we're on the latest version.

During review, it was moved below the case for a security update, when a
dep is already up-to-date but also _not_ vulnerable.

However, there's yet another case, which is where a dep is up-to-date
and _also_ vulnerable. I was surprised in
#6230 (comment)
to see that it spits out `no update needed as it's already up-to-date`
when I had explicitly passed in a security advisor marking the latest
version as vulnerable. It wasn't clear to me if the `dry-run` was
prematurely bailing before checking for vulnerable versions. That was
for the `pub` ecosystem, where both
`checker.lowest_resolvable_security_fix_version` and
`checker.latest_resolvable_version` return the same version.

The very next `if` block handles the case when the dep _is_ vulnerable.
So by moving this short-circuit below that, it will still short circuit
for the common case where a version is not vulnerable and on latest.

For reference, here's pre-change run:
```
[dependabot-core-dev] ~/dependabot-core $ SECURITY_ADVISORIES='[{"dependency-name":"buffer","patched-versions":[],"unaffected-versions":[],"affected-versions":["<= 2.0.0"]}]' bin/dry-run.rb pub dart-lang/pub-dev --dir "/app" --cache=files --dep="buffer" # direct dep
warning: parser/current is loading parser/ruby31, which recognizes3.1.3-compliant syntax, but you are running 3.1.2.
Please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
=> reading cloned repo from /home/dependabot/dependabot-core/tmp/dart-lang/pub-dev
=> parsing dependency files
I, [2022-12-04T22:26:35.101963 #38484]  INFO -- : Failed to infer the flutter version. Attempting to use latest stable release.
I, [2022-12-04T22:26:35.102107 #38484]  INFO -- : Cloning the flutter repo https://github.com/flutter/flutter.
I, [2022-12-04T22:27:40.833227 #38484]  INFO -- : Checking out Flutter version stable
I, [2022-12-04T22:27:46.471869 #38484]  INFO -- : Running `flutter doctor` to install artifacts and create flutter/version.
I, [2022-12-04T22:29:39.504293 #38484]  INFO -- : Running `flutter --version`
I, [2022-12-04T22:29:40.773354 #38484]  INFO -- : Installed the Flutter SDK version: 3.3.9 with Dart 2.18.5.
=> updating 1 dependencies: buffer

=== buffer (1.1.1) (vulnerable 🚨)
 => checking for updates 1/1
I, [2022-12-04T22:29:41.511587 #38484]  INFO -- : Failed to infer the flutter version. Attempting to use latest stable release.
I, [2022-12-04T22:29:41.511704 #38484]  INFO -- : Checking out Flutter version stable
I, [2022-12-04T22:29:42.735602 #38484]  INFO -- : Running `flutter doctor` to install artifacts and create flutter/version.
I, [2022-12-04T22:29:45.652134 #38484]  INFO -- : Running `flutter --version`
I, [2022-12-04T22:29:46.743673 #38484]  INFO -- : Installed the Flutter SDK version: 3.3.9 with Dart 2.18.5.
 => latest available version is 1.1.1
    (no update needed as it's already up-to-date)
🌍 Total requests made: '0'
```

And here's post-change run:
```
[dependabot-core-dev] ~/dependabot-core $ SECURITY_ADVISORIES='[{"dependency-name":"buffer","patched-versions":[],"unaffected-versions":[],"affected-versions":["<= 2.0.0"]}]' bin/dry-run.rb pub dart-lang/pub-dev --dir "/app" --cache=files --dep="buffer" # direct dep
warning: parser/current is loading parser/ruby31, which recognizes3.1.3-compliant syntax, but you are running 3.1.2.
Please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
=> reading cloned repo from /home/dependabot/dependabot-core/tmp/dart-lang/pub-dev
=> parsing dependency files
I, [2022-12-04T22:44:40.544797 #39357]  INFO -- : Failed to infer the flutter version. Attempting to use latest stable release.
I, [2022-12-04T22:44:40.544922 #39357]  INFO -- : Checking out Flutter version stable
I, [2022-12-04T22:44:41.954572 #39357]  INFO -- : Running `flutter doctor` to install artifacts and create flutter/version.
I, [2022-12-04T22:44:45.209362 #39357]  INFO -- : Running `flutter --version`
I, [2022-12-04T22:44:46.159503 #39357]  INFO -- : Installed the Flutter SDK version: 3.3.9 with Dart 2.18.5.
=> updating 1 dependencies: buffer

=== buffer (1.1.1) (vulnerable 🚨)
 => checking for updates 1/1
I, [2022-12-04T22:44:46.554930 #39357]  INFO -- : Failed to infer the flutter version. Attempting to use latest stable release.
I, [2022-12-04T22:44:46.555046 #39357]  INFO -- : Checking out Flutter version stable
I, [2022-12-04T22:44:47.709661 #39357]  INFO -- : Running `flutter doctor` to install artifacts and create flutter/version.
I, [2022-12-04T22:44:51.640507 #39357]  INFO -- : Running `flutter --version`
I, [2022-12-04T22:44:52.616051 #39357]  INFO -- : Installed the Flutter SDK version: 3.3.9 with Dart 2.18.5.
 => latest available version is 1.1.1
 => there is no available non-vulnerable version
    (no update needed as it's already up-to-date)
🌍 Total requests made: '0'
```

Note that the output above is slightly confusing because it's _not_
doing a security update only, but rather a normal update with a
specified security advisory... so I think it's fine for debugging
purposes that we get the information that there's no non-vulnerable
version, and also that from a version-checking perspective there's no
available update, regardless of vulnerability.
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.

3 participants