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

260 Parallelize multiple source fetch requests #264

Merged

Conversation

schuylermartin45
Copy link
Collaborator

@schuylermartin45 schuylermartin45 commented Dec 4, 2024

We now use futures to download multiple source files concurrently.

Also introduces a --dry-run mode that dumps the recipe file to STDOUT instead of overwriting the recipe file.

Putting this PR up as a Draft until I can figure out why some of the bump_recipe_cli tests are significantly slower after this change (causing the tests to take 4 seconds to 35 seconds). That issue is now tracked in #265 as it appears to have no known user impact and is tied to the test environment.

@schuylermartin45 schuylermartin45 linked an issue Dec 4, 2024 that may be closed by this pull request
@schuylermartin45
Copy link
Collaborator Author

schuylermartin45 commented Dec 4, 2024

The test_bump_recipe_cli() tests that reference

  • gsm-amzn2-aarch64_version_bump.yaml
  • libprotobuf_version_bump.yaml

cause the slow down. Each of these cases takes ~30 seconds to complete. They both have multiple sources but they are fully mocked.

@schuylermartin45
Copy link
Collaborator Author

The slow down goes away if I remove pyfakefs as a test fixture for test_bump_recipe_cli()

@schuylermartin45
Copy link
Collaborator Author

Upgrading to pyfakefs to 5.7.2 seems to help, but it could be transient. Some runs take 4 seconds, other runs still take 30+ seconds.

@schuylermartin45
Copy link
Collaborator Author

Reverted back to the older version, definitely transient. I just didn't run enough instances to see a shorter runtime.

@schuylermartin45
Copy link
Collaborator Author

Can't reproduce this slow down in a real (non-test) environment. Will spin-off this issue to another ticket, as it does not affect users.

@schuylermartin45 schuylermartin45 marked this pull request as ready for review December 5, 2024 15:49
@schuylermartin45 schuylermartin45 requested a review from a team as a code owner December 5, 2024 15:50
@schuylermartin45 schuylermartin45 merged commit 69d7184 into main Dec 5, 2024
13 checks passed
@schuylermartin45 schuylermartin45 deleted the 260-parallelize-multiple-source-fetch-requests branch December 5, 2024 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parallelize multiple source fetch requests
2 participants