Repository navigation
Fix benchmark workflow to test all three versions simultaneously with complete verification - #211
Conversation
…tput formatting Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
…arios Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
…sting Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
…ling Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
3つのバージョンすべての動作確認と最適化が完了しました (コミット: 06ebc0c) テスト結果: ✅ Original版 (dictsqlite/) - 同期専用
✅ dictsqlite_v2版 (Rust拡張 v2.0.6) - 同期/非同期対応
✅ Beta v2版 (dictsqlite-fastest) - 非同期専用
パフォーマンスランキング: 実施した最適化:
|
Co-authored-by: harumaki4649 <83683593+harumaki4649@users.noreply.github.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
1 similar comment
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Pull request overview
This pull request corrects version labeling inconsistencies in the benchmark workflow and enhances the testing infrastructure to properly compare three DictSQLite versions simultaneously with optimized sync/async tests and bilingual output formatting.
Key Changes
- Fixed all "v4.1" references to "dictsqlite_v2" throughout benchmark scripts and workflow files
- Enhanced output formatting with bilingual labels (Japanese/English) and emoji icons for better readability
- Added advanced benchmarking capabilities with memory profiling and extended test scenarios
- Implemented improved version loading mechanism to handle import conflicts between Original, dictsqlite_v2, and Beta v2 versions
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
others/benchmark/benchmark_all_versions.py |
Updated version references, enhanced output formatting with bilingual labels and emojis, refactored import mechanism to load all three versions without conflicts, changed v2 benchmark from async to sync mode |
others/benchmark/run_benchmark.py |
Corrected docstrings and print statements to replace "v4.1" with "dictsqlite_v2版" |
others/benchmark/advanced_benchmark.py |
New file adding memory profiling, extended benchmark scenarios (large datasets, concurrent writes, update/delete operations, mixed workloads), and JSON result export |
.github/workflows/benchmark.yml |
Updated version labels in workflow comments to clarify sync/async capabilities of each version |
others/benchmark/results/versions/*/benchmark.csv |
Generated benchmark results from test runs (data files) |
others/benchmark/results/version_history.json |
Added new benchmark run history entries (generated data) |
others/benchmark/results/advanced_benchmark_results.json |
New empty results file for advanced benchmark output (generated data) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| finally: | ||
| if os.path.exists(db_path): | ||
| os.unlink(db_path) |
There was a problem hiding this comment.
Duplicate finally blocks detected. Lines 371-373 and 374-376 are identical, causing the second finally block to be unreachable. Remove one of these duplicate finally blocks.
| finally: | |
| if os.path.exists(db_path): | |
| os.unlink(db_path) |
| except: | ||
| pass |
There was a problem hiding this comment.
Bare except: clause should specify exception type. Replace with except Exception: to avoid catching system-exiting exceptions like KeyboardInterrupt and SystemExit.
| except: | ||
| pass |
There was a problem hiding this comment.
Bare except: clause should specify exception type. Replace with except Exception: to avoid catching system-exiting exceptions like KeyboardInterrupt and SystemExit.
| try: | ||
| from dictsqlite.main import DictSQLite # Original (sync only) | ||
| # Import Original version directly from source to avoid conflicts with installed packages | ||
| import importlib.util |
There was a problem hiding this comment.
importlib.util is imported at the module top (line 15) but imported again here at line 49. The import at line 50 for importlib.machinery is fine since it wasn't previously imported. Consider removing the redundant import importlib.util at line 49.
| import importlib.util |
Problem
The benchmark workflow incorrectly referenced "v4.1" instead of "dictsqlite_v2" and lacked actual performance verification. The issue requested proper comparison of three versions with optimized tests and improved output formatting.
Changes
Version Label Corrections
benchmark_all_versions.py,run_benchmark.py, andbenchmark.ymldictsqlite/(sync-only, sqlite3)Output Format Enhancement
Before:
After:
Build Process Integration
build.shto build dictsqlite_v2 v2.0.6Complete Three-Version Testing
All three versions now work simultaneously with proper import handling:
Original版 (dictsqlite/):
dictsqlite_v2版 (Rust extension v2.0.6):
Beta v2版 (dictsqlite-fastest):
Import Resolution
Advanced Benchmarking
Added
advanced_benchmark.pywith:MemoryProfilerclassCode Quality
exceptclauses →except ExceptionTesting
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.