fix: batch pip installs in bootstrap for faster installs#4530
fix: batch pip installs in bootstrap for faster installs#4530Manvi2402 wants to merge 8 commits into
Conversation
| ### Fixed | ||
|
|
||
| - `opentelemetry-instrumentation`: Batch all packages into a single `pip install` call in `bootstrap.py` to improve performance | ||
| ([#4484](https://github.com/open-telemetry/opentelemetry-python-contrib/issues/4484)) |
There was a problem hiding this comment.
This needs to be updated to link to the PR
There was a problem hiding this comment.
Updated the changelog to link to the PR instead of the issue. Thanks for the review.
| "-m", | ||
| "pip", | ||
| "install", | ||
| "-U", |
There was a problem hiding this comment.
Thanks For your contribution !
Can you also add to the opentelemetry-bootstrap -a install command a flag support to disable the -U of pip install?
In a fresh environment (e.g., Docker build), there's nothing to upgrade, so the upgrade check is pure overhead
There was a problem hiding this comment.
Thanks for the suggestion. Added a --no-upgrade flag to disable the -U option for fresh environments like Docker builds.
MikeGoldsmith
left a comment
There was a problem hiding this comment.
Nice optimization — batching pip installs is a cool thing to see.
A few things to tidy up:
-
Error message broken for batched packages —
f'{msg} for package "{packages}"'will print the list repr (e.g.for package "['pkg1', 'pkg2']"). Should join them, e.g.f'{msg} for packages "{", ".join(packages)}"'. -
_syscallwrapper is fragile —if packages:is falsy for[], which would fall through tofunc()with no args and crash. Currently protected by theif libs:guard in_run_install, but the wrapper shouldn't rely on callers to not pass empty lists. -
--no-upgradeflag — this is a separate feature from the batching change. Consider splitting it into its own PR or at minimum mentioning it in the CHANGELOG entry. -
CHANGELOG section — this is a performance improvement, not a bug fix. Should be under a different section.
df930c4 to
c711a18
Compare
|
Hi @MikeGoldsmith , all review comments addressed! Fixed error message to use ', '.join(packages) instead of list repr |
|
Thanks for the PR! Just a heads-up: we no longer update Please add the appropriate changelog fragment for this change instead of editing |
Description
Previously,
_run_installinbootstrap.pycalledpip installonce perpackage in a loop, causing multiple subprocess spawns, pip loads, and
dependency resolution passes. For a typical project with ~8 instrumentors,
this took 60-120 seconds.
This change batches all packages into a single
pip installcall, reducingit to 15-25 seconds (4-6x speedup).
Fixes #4484
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
python -m py_compileDoes This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.