Repository navigation
Parallelize benchmark workflow builds, fix security vulnerabilities, and improve version clarity with graphs and optimized sync/async testing - #209
Merged
Conversation
…handling Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
- Add wheel file count validation to prevent ambiguous installations - Improve test script to properly validate directory creation - Address code review feedback Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
- Update actions/download-artifact from v4 to v4.1.3 (fixes arbitrary file write vulnerability) - Update actions/upload-artifact from v4 to v4.4.3 (latest stable) - Update actions/setup-python from v3 to v5 (latest stable) - All dependencies verified to have no known vulnerabilities Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix issues with benchmark.yml workflow
Parallelize benchmark workflow builds and fix security vulnerabilities
Dec 6, 2025
- Add clear version mapping table showing which label corresponds to which code - Add graph generation to benchmark_all_versions.py (bar chart and speedup chart) - Update fast_comprehensive_benchmark.py with version mapping - Clarify Original版, Fastest版, Beta版, Beta v2版, v4.1版 in output Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
Contributor
Author
バージョンマッピングを明確化し、グラフ生成機能を追加しました (commit abacc53) 修正内容:
出力例: |
Remove misplaced code block that was duplicating save_version_results logic Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
- Remove unused imports (matplotlib.font_manager, seaborn) - Fix baseline calculation to avoid ValueError with empty list - Fix speedup indexing logic to correctly map versions to their speedup values Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
Copilot
AI
changed the title
Parallelize benchmark workflow builds and fix security vulnerabilities
Parallelize benchmark workflow builds, fix security vulnerabilities, and add version clarity with graphs
Dec 6, 2025
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| try: | ||
| plt.rcParams['font.family'] = 'sans-serif' | ||
| plt.rcParams['font.sans-serif'] = ['DejaVu Sans', 'Arial', 'Noto Sans CJK JP'] | ||
| except Exception: |
Check notice
Code scanning / Bandit
Try, Except, Pass detected.
- Change "Beta版のバージョン選択" to "テスト対象の選択" - Move "all" to first option (default) since it tests main versions - Add detailed comments explaining what each option does: - all: Tests 3 main versions (Original, Beta v2, v4.1) - v1-v4: Tests dictsqlite-fastest Beta revisions - Add runtime display showing what will be tested - Update all echo messages to clarify which specific version is being tested Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
Contributor
Author
ワークフローの入力オプションを明確化しました (commit e667976) 主な変更:
# テスト対象の選択オプション:
# - all: 3つの主要バージョンを比較 (Original, Beta v2, v4.1)
# → Original版 (dictsqlite/)
# → Beta v2版 (dictsqlite-fastest beta v2)
# → v4.1版 (dictsqlite_v4.1 Rust実装)
#
# - v1, v2, v3, v4: dictsqlite-fastest Betaの各リビジョンをテスト
# → v1: dictsqlite-fastest beta v1
# → v2: dictsqlite-fastest beta v2
# → v3: dictsqlite-fastest beta v3-alpha
# → v4: dictsqlite-fastest beta v4-final
これで、ワークフローの入力オプションと実際のテスト内容が一致し、わかりやすくなりました。 |
Explain that BETA_VERSION is used for all test targets including "all", not just beta versions Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
Copilot
AI
changed the title
Parallelize benchmark workflow builds, fix security vulnerabilities, and add version clarity with graphs
Parallelize benchmark workflow builds, fix security vulnerabilities, and improve version clarity with graphs and workflow input options
Dec 6, 2025
- Update benchmark_all_versions.py to compare: Original, dictsqlite_v2, and Beta v2 - Remove v4.1 from "all" mode as requested - Add optimized sync/async tests for dictsqlite_v2 (uses aset/aget async API) - Update workflow header comments to reflect new comparison - Update version mapping messages in benchmark output - dictsqlite_v2 now tests both sync and async operations properly Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
Contributor
Author
"all"モードを更新しました (commit 6a6e4df) 以下の3つのバージョンを比較するように変更:
v4.1を削除し、それぞれに最適化したテストを実装:
ベンチマークでは以下をテスト:
グラフも3バージョンの比較に更新されます。 |
Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
Copilot
AI
changed the title
Parallelize benchmark workflow builds, fix security vulnerabilities, and improve version clarity with graphs and workflow input options
Parallelize benchmark workflow builds, fix security vulnerabilities, and improve version clarity with graphs and optimized sync/async testing
Dec 6, 2025
Contributor
There was a problem hiding this comment.
Pull request overview
This PR comprehensively improves the benchmark workflow by addressing 5 reported issues: parallelizing Rust extension builds for 50% faster build times, clarifying version naming and mappings with runtime displays, preventing CSV generation errors through pre-created directories and validation, optimizing tests for each version's sync/async capabilities, and fixing a critical security vulnerability in actions/download-artifact@v4.
- Parallelized Rust extension builds using matrix strategy, reducing build time from 10min to 5min
- Updated "all" mode to compare Original, dictsqlite_v2 (Rust v2.0.6), and Beta v2 versions with optimized sync/async testing
- Added comprehensive version mapping documentation, runtime displays, and automatic graph generation with matplotlib
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/benchmark.yml |
Parallelized Rust builds with new build-rust-extensions job, updated GitHub Actions to secure versions (download-artifact v4.1.3, upload-artifact v4.4.3, setup-python v5), added version display step, improved error handling with explicit checks, and added CSV validation |
others/benchmark/benchmark_all_versions.py |
Replaced v4.1 with dictsqlite_v2 in "all" mode comparison, added async benchmark functions for dictsqlite_v2 using aset/aget API, implemented matplotlib-based graph generation (bar chart and speedup comparison), updated version mapping documentation, and added comprehensive error handling |
others/benchmark/fast_comprehensive_benchmark.py |
Added version mapping table to output, though the mapping content needs updating to match the actual "all" mode versions tested |
BENCHMARK_WORKFLOW_COMPLETION_REPORT.md |
New comprehensive completion report documenting all 5 issues addressed, implementation details, testing results, and expected performance improvements |
.github/workflows/SECURITY_SUMMARY.md |
New security documentation detailing the fixed arbitrary file write vulnerability in actions/download-artifact and all GitHub Actions version updates |
.github/workflows/BENCHMARK_IMPROVEMENTS.md |
New detailed improvement documentation explaining parallelization, version clarity, CSV error prevention, error handling, and artifact-based build system |
.github/workflows/test_benchmark_workflow.sh |
New local testing script with 15 validation checks covering directory structure, file structure, version information, Python syntax, YAML syntax, and directory creation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+582
to
+598
| version_labels = [] | ||
| if ORIGINAL_AVAILABLE: | ||
| version_labels.append('Original版') | ||
| if BETA_V2_AVAILABLE: | ||
| version_labels.append('Beta v2版') | ||
| if V2_AVAILABLE: | ||
| version_labels.append('dictsqlite_v2版') | ||
|
|
||
| # Plot each version's speedups across tests | ||
| for version_idx, version_label in enumerate(version_labels): | ||
| version_speedups = [] | ||
| for test_speedup_list in speedups: | ||
| # Get speedup for this version from this test | ||
| if version_idx < len(test_speedup_list): | ||
| version_speedups.append(test_speedup_list[version_idx]) | ||
| else: | ||
| version_speedups.append(0) |
There was a problem hiding this comment.
[nitpick] The order of versions in the speedup graph's version_labels list doesn't match the order in which speedups are appended to test_speedups. This causes incorrect label-to-data mapping in the plot.
Issue:
test_speedupsis built in order: ORIGINAL → BETA_V2 → V2 (lines 569-574)version_labelsis built in order: ORIGINAL → BETA_V2 → V2 (lines 583-588)
While the order currently matches, the logic at line 595-598 assumes the indices align, but there's a subtle bug: if one version is unavailable (e.g., ORIGINAL_AVAILABLE is False), the indices will be misaligned.
Example scenario:
- If ORIGINAL_AVAILABLE=False, BETA_V2_AVAILABLE=True, V2_AVAILABLE=True:
test_speedups= [beta_v2_speedup, v2_speedup] (2 elements)version_labels= ['Beta v2版', 'dictsqlite_v2版'] (2 elements)version_speedupsfor version_idx=0 gets test_speedup_list[0] (beta_v2, correct)version_speedupsfor version_idx=1 gets test_speedup_list[1] (v2, correct)
Actually, the code is correct! The conditional checks ensure both lists are built in the same order. However, the code would be more maintainable if it explicitly tracked version-to-speedup mappings using a dictionary rather than relying on implicit ordering.
Suggested change
| version_labels = [] | |
| if ORIGINAL_AVAILABLE: | |
| version_labels.append('Original版') | |
| if BETA_V2_AVAILABLE: | |
| version_labels.append('Beta v2版') | |
| if V2_AVAILABLE: | |
| version_labels.append('dictsqlite_v2版') | |
| # Plot each version's speedups across tests | |
| for version_idx, version_label in enumerate(version_labels): | |
| version_speedups = [] | |
| for test_speedup_list in speedups: | |
| # Get speedup for this version from this test | |
| if version_idx < len(test_speedup_list): | |
| version_speedups.append(test_speedup_list[version_idx]) | |
| else: | |
| version_speedups.append(0) | |
| # Build list of available versions as (key, label) | |
| available_versions = [] | |
| if ORIGINAL_AVAILABLE: | |
| available_versions.append(('ORIGINAL', 'Original版')) | |
| if BETA_V2_AVAILABLE: | |
| available_versions.append(('BETA_V2', 'Beta v2版')) | |
| if V2_AVAILABLE: | |
| available_versions.append(('V2', 'dictsqlite_v2版')) | |
| # Plot each version's speedups across tests | |
| for version_key, version_label in available_versions: | |
| version_speedups = [] | |
| for test_speedup_dict in speedups: | |
| # Get speedup for this version from this test dict | |
| version_speedups.append(test_speedup_dict.get(version_key, 0)) |
Comment on lines
+648
to
+658
| print("\n以下の3つのバージョンを比較します:") | ||
| print("\n 1. Original版 (DictSQLite)") | ||
| print(" - ソースコード: dictsqlite/") | ||
| print(" - 実装: 標準sqlite3ベース") | ||
| print("\n 2. Fastest版 (DictSQLite-Fastest)") | ||
| print(" - ソースコード: others/beta-versions/dictsqlite-fastest/") | ||
| print(" - 実装: APSWベース（高性能版）") | ||
| print("\n 3. Beta版 (DictSQLite-Fastest Beta)") | ||
| print(" - ソースコード: others/beta-versions/dictsqlite-fastest/beta/") | ||
| print(" - 実装: メモリ最適化版（LRUキャッシュ搭載）") | ||
| print("\n" + "="*80) |
There was a problem hiding this comment.
The version mapping documentation in fast_comprehensive_benchmark.py is inconsistent with the actual versions being tested. According to the PR description and the changes in benchmark_all_versions.py, the "all" mode should compare:
- Original版 (dictsqlite/)
- dictsqlite_v2版 (dictsqlite_v2/dictsqlite/) - Rust extension v2.0.6
- Beta v2版 (dictsqlite-fastest beta v2)
However, this file still shows the old mapping:
- Original版
- Fastest版 (dictsqlite-fastest)
- Beta版 (dictsqlite-fastest beta)
This doesn't align with the PR's stated goal of updating version clarity. The documentation should match what benchmark_all_versions.py actually tests.
| """Main benchmark execution.""" | ||
| print("=" * 70) | ||
| print("DictSQLite Comprehensive Benchmark") | ||
| print("Original vs Beta v2 vs v4.1") |
There was a problem hiding this comment.
The title line says "Original vs Beta v2 vs v4.1" but this is outdated. According to the PR description and the actual code changes, this benchmark now compares:
- Original (dictsqlite/)
- dictsqlite_v2 (Rust extension v2.0.6)
- Beta v2 (dictsqlite-fastest beta v2)
The title should be updated to "Original vs dictsqlite_v2 vs Beta v2" to accurately reflect what's being tested.
Suggested change
| print("Original vs Beta v2 vs v4.1") | |
| print("Original vs dictsqlite_v2 vs Beta v2") |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The benchmark workflow had 5 issues: sequential builds causing 2x slowdown, unclear version naming (dictsqlite_v2 version not documented), CSV generation errors, non-optimized version testing, and a critical security vulnerability in
actions/download-artifact@v4.Changes
Parallelized Rust Extension Builds
build-rust-extensionsjob with matrix strategy for v4.1 and v2benchmarkjob via upload/downloadVersion Clarity
"all" Mode Version Comparison Update
Optimized Sync/Async Testing
aset/agetAPI (Rust with full async support)Workflow Input Options Clarity
all: Tests 3 main versions (Original, dictsqlite_v2, Beta v2)v1-v4: Tests dictsqlite-fastest Beta revisions (v1, v2, v3-alpha, v4-final)Graph Generation
version_comparison_bar.png- Bar chart comparing Original, dictsqlite_v2, and Beta v2 across all testsversion_comparison_speedup.png- Speedup comparison line chart showing relative performanceothers/benchmark/results/graphs/directoryCSV Error Prevention
results/{versions,graphs,comparisons}directories|| { echo "❌ Error"; exit 1; }Wheel Installation Safety
Security Fixes
actions/download-artifact:v4→v4.1.3(patches arbitrary file write CVE)actions/upload-artifact:v4→v4.4.3actions/setup-python:v3→v5Testing
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.