ci: add import profiler check across monorepo#17657
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a new import_profile session to the noxfile.py.j2 template to measure and ensure import times remain below defined thresholds. Feedback points out that because this template is used in standalone repositories outside of the monorepo, referencing a relative path to the monorepo's root scripts directory will cause default nox runs to fail in those environments. It is recommended to check for the script's existence and gracefully skip the session if it is missing, while also using pathlib.Path for cleaner path handling.
There was a problem hiding this comment.
I left a couple comments. It would be good to clarify the goal of this check though.
What counts as a failure? Can we calculate an import time diff before and after the target changes, instead of just reporting an absolute value? How are developers intended to interact with this?
| ;; | ||
| *) | ||
| nox -s ${TEST_TYPE} | ||
| retval=$? |
There was a problem hiding this comment.
This PR doesn't touch any packages, so it's hard to tell what the action will look like. Can you temporarily touch some files to test it out?
There was a problem hiding this comment.
We should also see how long the new check would take if all packages were updated. In #17438, I added a new unit_test:all_packages tag, that will run against all packages when added. Maybe we should support that here too
It's possible we would only want to profile a few key packages instead of all of them. Most generated libraries should be very similar
There was a problem hiding this comment.
I agree with Daniel on a couple of performance issues:
- I would like to see this thing run so that I can understand what it will produce and whether output appears as I would expect. Please temporarily touch some files to trigger the profiler to run against them. As an example: add a blank line right after the license in this file in google-cloud-compute and a similar change in at least one other package. That will trigger the profiler to run (without requiring you to go back and undo any changes to
google-cloud-compute, etc).
- If you already did this and have reverted your change, please point to the commit that you used to trigger the profiler (we should be able to check the CI/CD results for that commit).
Note
In case you've never done it... clicking on the x OR checkmark next to the commit number grants access to the test results for previous commits:
- If, as Daniel suggests, we run the profiler against all packages, it would be useful to know how long that CI/CD check will take. We have 240 generated packages and if we regen and profile all of them we don't wanna find out later that it takes hours. Do we have benchmark results to give us an order of magnitude for how long it takes?
There was a problem hiding this comment.
I've added a temporary commit (8a2d75b) that adds a blank line to google-cloud-compute and google-api-core so you can see the profiler run on a couple of real packages and review the output.
Regarding the benchmark on all packages, we actually already support running the profiler against all packages in this PR! Similar to how the unit_test:all_packages label works, there's logic in .github/workflows/import-profiler.yml that checks for an import_profile:all_packages label.
If we want to see how long it takes across all ~240 packages to get an order of magnitude, we can simply apply the import_profile:all_packages label to this PR and let it run.
hebaalazzeh
force-pushed
the
feat/import-profiler-ci
branch
from
113b3d5 to
9472ab6
Compare
|
/gemini |
|
I reviewed the results of the profiler run. It appears as though it is partially successful and yet, halfway through each run for google-cloud-compute and google-api-core an error message appears Later in the same stretch we see some results that indicate a failure but do so in a way that is non-intuitive: If the built in profiler injects that line in its results, there may be nothing we can do about it, none-the-less, what we want to happen is for our process in its entirety to complete with either a success flag OR fail flag and have a message at the end. Something like this at the end of your profiler script could do the trick of signaling to the CI/CD workflow that the script resulted in a success OR failure result: |
|
Thanks for the detailed review @chalmerlowe! You raised some excellent points. I've pushed some fixes and ran some local benchmarks to answer your questions.
Regarding your question about how long it will take to run against all 240 packages: You were absolutely right to be concerned. Based on the profiler logs, 10 iterations of google-cloud-compute takes roughly ~2.5 to 4 minutes depending on the GitHub runner. (Note: This is measuring the unoptimized baseline. Although the PEP 0810 lazy-loading implementation was just released in gapic-generator v1.37.0 via PR #17600, google-cloud-compute has not been regenerated yet to pick up those changes. Once compute is regenerated, this runtime will drop dramatically!) Since our bash script evaluates the packages sequentially inside a single GitHub Actions worker, running across all 240 packages with these unoptimized eager imports would take between 4 to 10 hours. This means a full monorepo run would almost certainly hit the hard 6-hour GitHub Actions timeout. For now, I've left the import_profile:all_packages label functionality in place for rare/partial uses, but if we ever want to mandate a monorepo-wide baseline generation, we'll need to refactor the workflow to use a parallelized matrix strategy instead of a sequential bash loop. Let me know if you have any other questions! |
|
I have been thinking more about the math behind this work and I am not convinced we are doing this correctly. I worry about statistical noise. Statistical Noise & Sample SizeLet's consider this result from one of the runs in this PR: In any calculation, the highest datapoint is the 100th percentile. In this case our I suspect that in this case to calculate a P99, our benchmarking tool is extrapolating (guessing) data points that do not exist. It is generating a statistical hallucination. The 99th percentile in 100 datapoints is the second datapoint from the tail end of the distribution. I.e. if you sort the latencies from lowest to highest the 99th percentile would be datapoint number 99 out of 100. Which means that if the second highest datapoint is even a bit higher than expected it can have a massive effect on the calculated results. This IBM article talks about a similar issue where they were considering latency in one of their processes and concluded that with even 300 samples it was likely not enough for what they wanted to measure:
https://www.ibm.com/support/pages/why-p99-latency-metrics-are-unreliable-low-traffic-workloads RECOMMENDATION: If we want to limit ourselves to 10 iterations, we should really only be comparing P50 or the CPU PinningLooking at this: |
chalmerlowe
left a comment
chalmerlowe
left a comment
There was a problem hiding this comment.
Got some questions on whether our approach and or algorithms are correct.
- needless repetition
- inability to rely on P99 values
- module name handling
|
Benchmarking is notoriously tricky: One issue is caching at multiple levels. 1. The OS Page CachePython profiling tools (time.perf_counter(), importlib, etc.) measure time, but they cannot control or reset the Operating System's file cache (Page Cache). What happens in the script:The script runs the Baseline (10 iterations). It imports the package and its dependencies. The OS reads these files from disk and keeps them in RAM (cache). When the PR version runs, many of the files (especially common dependencies like protobuf, grpc, or google-api-core) are already sitting in RAM from the Baseline run Result:the PR run might appear artificially faster than the Baseline simply because it got to read from RAM (Warm) while the Baseline had to read from disk (Cold). This can hide performance regressions! 2. The Python Bytecode Cache (.pyc)What happens in the script:The author tried to account for this in profiler.py with clean_bytecode(), but there might be a bug in their logic! The Code: # profiler.py
for root, dirs, files in os.walk('.'):
dirs[:] = [d for d in dirs if not d.startswith('.')] # This line skips hidden dirs!In ci/run_single_test.sh, the virtual environment is created as .venv-profiler (a hidden directory). Because clean_bytecode() skips directories starting with a dot, it will NOT delete the pycache files inside the virtualenv. Result: Dependencies installed in the venv will keep their pre-compiled bytecode across runs, meaning you aren't truly measuring a "Cold" Python startup.Recommendations for consideration:If we want reliable results, we have two main options: Option A: Focus ONLY on "Warm" Starts. How: Run the profiler 11 times, but discard the first run as a "burn-in" run to warm up the cache. Measure only runs 2-11. This ensures both Baseline and PR are compared on equal "warm" footing. Option B: True "Cold" Starts (Harder in CI) Cold starts might be necessary if we want to simulate users loading our packages in a serverless environment or Google Cloud Functions. But then again, warm may be satisfactory if we are predominantly worried less on total time to start versus a change to the time to start. (i.e. placing less emphasis on < 5000 ms and more on the diff between PR and main is < x%) |
… module discovery
|
Thanks for the follow-up, @chalmerlowe! I've pushed an additional commit to address these points:
Let me know if there is anything else that needs to be addressed. |
|
Do we know what the ceiling of this presubmit looks like? i.e. if this check is run for all the packages? |
…ds, and tempfiles
…al 1-2hr presubmits
chalmerlowe
left a comment
chalmerlowe
left a comment
There was a problem hiding this comment.
A nit and a fundamental reservation about relying on P99.
Co-authored-by: Chalmer Lowe <chalmerlowe@google.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an import profiling step (import_profile) to the CI pipeline to benchmark Python package import times and detect regressions against a baseline commit. It also updates the profiler script to support auto-detecting modules from package names, discarding the first iteration as a burn-in run, and enforcing absolute and relative performance thresholds. The review feedback highlights three critical issues: 1) Running git checkout directly in the CI script can carry over local modifications or fail, so using git worktree and supporting fork remotes is recommended. 2) Including .venv-profiler in the bytecode cleanup will delete compiled bytecode for all dependencies, skewing the benchmark results. 3) A missing import for importlib.util in find_module_from_package causes a silent NameError that breaks the module auto-detection fallback logic.
| if [ -n "${TARGET_BRANCH}" ]; then | ||
| # Suppress errors if branch isn't found | ||
| BASELINE_COMMIT=$(git merge-base HEAD "origin/${TARGET_BRANCH:-main}" 2>/dev/null || true) | ||
| if [ -n "${BASELINE_COMMIT}" ]; then | ||
| echo "Checking out baseline commit ${BASELINE_COMMIT}..." | ||
| git checkout ${BASELINE_COMMIT} | ||
| if [ -f setup.py ] || [ -f pyproject.toml ]; then | ||
| pip install -e . | ||
| python ${PROFILER_SCRIPT} --package ${PACKAGE_NAME} --iterations 11 --csv "${BASELINE_CSV}" | ||
| else | ||
| echo "setup.py/pyproject.toml not found on baseline. Skipping baseline generation." | ||
| fi | ||
| git checkout - | ||
| else | ||
| echo "Could not find baseline commit for origin/${TARGET_BRANCH:-main}. Skipping baseline generation." | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Running git checkout ${BASELINE_COMMIT} directly in the current working directory will either fail (if there are conflicting changes in the PR) or carry over the PR's local modifications to the baseline checkout. In both cases, the baseline profiling will run on the PR branch's code instead of the clean baseline commit, resulting in a 0ms difference and silently bypassing the regression check.
To fix this, use git worktree to check out a clean copy of the baseline commit in a temporary directory, run the baseline profile there, and then clean up the worktree. This ensures a completely clean baseline measurement without affecting the current working directory or risking stash conflicts.
Additionally, since pull requests can originate from forked repositories, ensure that the target branch is fetched from the correct remote (which may be 'upstream' rather than 'origin') to correctly locate the baseline commit.
if [ -n "${TARGET_BRANCH}" ]; then
# Try upstream first (for forks), then origin
BASELINE_COMMIT=$(git merge-base HEAD "upstream/${TARGET_BRANCH}" 2>/dev/null || \
git merge-base HEAD "origin/${TARGET_BRANCH}" 2>/dev/null || \
git merge-base HEAD "${TARGET_BRANCH}" 2>/dev/null || true)
if [ -n "${BASELINE_COMMIT}" ]; then
echo "Checking out baseline commit ${BASELINE_COMMIT} in a temporary worktree..."
REPO_PREFIX=$(git rev-parse --show-prefix)
WORKTREE_DIR=$(mktemp -d)
rmdir "${WORKTREE_DIR}"
if git worktree add "${WORKTREE_DIR}" "${BASELINE_COMMIT}" 2>/dev/null; then
(
cd "${WORKTREE_DIR}/${REPO_PREFIX}"
if [ -f setup.py ] || [ -f pyproject.toml ]; then
pip install -e .
python "${PROFILER_SCRIPT}" --package "${PACKAGE_NAME}" --iterations 11 --csv "${BASELINE_CSV}"
fi
)
git worktree remove -f "${WORKTREE_DIR}"
else
echo "Failed to create git worktree for baseline. Skipping baseline generation."
fi
else
echo "Could not find baseline commit for ${TARGET_BRANCH:-main}. Skipping baseline generation."
fi
fiReferences
- When fetching a specific commit in a Git repository, consider that pull requests can originate from forked repositories, which might affect how the commit is fetched.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
4 participants
Overview
This PR integrates the import profiler script (introduced in #17467) into our automated CI pipeline.
Why this matters: Import times significantly impact CLI responsiveness and cold starts for Serverless products like Cloud Run and Cloud Functions. Ideally, library imports should stay under 500ms, and anything taking over 1 second is a target for optimization. This CI check helps us proactively track metrics like import latency, memory footprint, and code volume to prevent performance regressions on critical libraries.
The primary goal of this check is to track and enforce performance standards for package import times across the repository, especially following our recent work on lazy loading and cold-start optimizations.
By running this benchmark as a CI check with a defined dynamic differential failure threshold, we can programmatically prevent latency regressions in module initialization times before they are merged, ensuring downstream consumers aren't impacted by unexpectedly slow startup times.
Changes Included
.github/workflows/import-profiler.yml) that triggers on PRs and merge groups. The workflow is pinned specifically to Python 3.15.noxfile.py.j2) and forcing updates across 150+ packages, the CI script (ci/run_single_test.sh) natively handles the profiler. It automatically spins up a lightweight virtual environment, installs the target package, and runs the profiler.ci/run_single_test.shto checkoutHEAD^1(the main branch), generate a baseline CSV profile, and then diff it against the PR branch. If the Median (P50) import time of the PR degrades by >100ms compared to the baseline, the CI check will fail. (Note: Using Median instead of P99 ensures stability against intermittent CPU spikes on GitHub Action runners).setup.pyexists before attempting to profile, gracefully skipping directories that are not valid Python packages.Developer Interactions
If a developer fails this CI check due to an import latency regression, they can reproduce and debug it locally by running the profiler script with the
--cprofileflag (python scripts/import_profiler/profiler.py --package <their-package> --cprofile). This will generate a cProfile stack trace breakdown of the import time, allowing them to pinpoint exactly which new dependency or module initialization is causing the latency spike.Related PRs