Conversation
product-auto-label
bot
added
size: m
Summary of ChangesHello @chandra-siri, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request integrates microbenchmarks into the continuous integration system. It establishes a new Cloud Build pipeline that provisions a Google Compute Engine virtual machine, sets up the necessary environment, and executes a defined set of performance tests. This ensures that performance characteristics are regularly monitored and potential regressions are identified early in the development cycle. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces a new CI job for running micro-benchmarks, which is a valuable addition for performance tracking. The implementation is mostly solid, but I've found a few issues that need to be addressed. There's a critical issue in the Cloud Build configuration where the wrong script is being executed, and another bug related to handling the exit code. I've also suggested some improvements to the test execution script and a minor fix in the benchmark configuration file to enhance maintainability and test coverage.
cloudbuild/micro_benchmarks.yaml
Outdated
| # Execute the script on the VM via SSH. | ||
| # Capture the exit code to ensure cleanup happens before the build fails. | ||
| set +e | ||
| gcloud compute ssh ${_VM_NAME} --zone=${_ZONE} --internal-ip --ssh-key-file=/workspace/.ssh/google_compute_engine --command="ulimit -n {_ULIMIT}; COMMIT_SHA=${COMMIT_SHA} _ZONAL_BUCKET=${_ZONAL_BUCKET} _PR_NUMBER=${_PR_NUMBER} bash run_zonal_tests.sh" |
There was a problem hiding this comment.
The script being executed on the VM is run_zonal_tests.sh, but the script that was copied to the VM in the previous step is run_micro_benchmarks.sh. This will cause the wrong tests to be executed. You should execute run_micro_benchmarks.sh instead.
gcloud compute ssh ${_VM_NAME} --zone=${_ZONE} --internal-ip --ssh-key-file=/workspace/.ssh/google_compute_engine --command="ulimit -n {_ULIMIT}; COMMIT_SHA=${COMMIT_SHA} _ZONAL_BUCKET=${_ZONAL_BUCKET} _PR_NUMBER=${_PR_NUMBER} bash run_micro_benchmarks.sh"| gcloud compute instances delete "${_VM_NAME}" --zone=${_ZONE} --quiet | ||
|
|
||
| # Exit with the original exit code from the test script. | ||
| exit $$EXIT_CODE |
There was a problem hiding this comment.
The expression $$EXIT_CODE is incorrect. $$ expands to the process ID of the current shell, so this command will effectively try to exit with a code like 123$?. You should use $EXIT_CODE to correctly use the captured exit code from the test script.
exit $EXIT_CODE| pytest -vv -s \ | ||
| --log-format='%(asctime)s %(levelname)s %(message)s' \ | ||
| --log-date-format='%H:%M:%S' \ | ||
| --benchmark-json=output.json \ | ||
| -vv -s tests/perf/microbenchmarks/reads/test_reads.py::test_downloads_single_proc_single_coro |
There was a problem hiding this comment.
The pytest command has duplicated flags (-vv and -s). While this doesn't cause an error, it's redundant. Additionally, only a single test function is being run. Given that the benchmark configuration in config.yaml has been scaled down, it would be beneficial to run all tests in test_reads.py to get broader benchmark coverage in CI.
| pytest -vv -s \ | |
| --log-format='%(asctime)s %(levelname)s %(message)s' \ | |
| --log-date-format='%H:%M:%S' \ | |
| --benchmark-json=output.json \ | |
| -vv -s tests/perf/microbenchmarks/reads/test_reads.py::test_downloads_single_proc_single_coro | |
| pytest -vv -s \ | |
| --log-format='%(asctime)s %(levelname)s %(message)s' \ | |
| --log-date-format='%H:%M:%S' \ | |
| --benchmark-json=output.json \ | |
| tests/perf/microbenchmarks/reads/test_reads.py |
| - 1024 # 1GiB | ||
| chunk_sizes_mib: [100] | ||
| rounds: 10 | ||
| - 100 # 1GiB |
There was a problem hiding this comment.
The comment # 1GiB is no longer accurate since the file size has been changed to 100 MiB. Please update the comment to reflect the new value to avoid confusion.
- 100 # 100MiB
|
/gcbrun(5fc8fcd) |
|
/gcbrun(2f13bb8) |
|
/gcbrun(8594296) |
|
/gcbrun(bc6bf69) |
1 participant
feat: run benchmarks in CI