-
Notifications
You must be signed in to change notification settings - Fork 6
fix: Fix incomplete renaming of tool-specific methods, and imports in benchall.py. #5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,12 @@ | ||
| #!/usr/bin/env python3 | ||
|
|
||
| from assets.clp_s.main import clp_s_bench | ||
| from assets.clickhouse_native_json.main import clickhouse_native_json_bench | ||
| from assets.clp.main import clp_bench | ||
| from assets.clickhouse.main import clickhouse_bench | ||
| from assets.sparksql.main import sparksql_bench | ||
| from assets.parquet.main import parquet_bench | ||
| from assets.presto_parquet.main import presto_parquet_bench | ||
| from assets.zstandard.main import zstandard_bench | ||
| from assets.elasticsearch.main import elasticsearch_bench | ||
| from assets.clp_presto.main import clp_presto_bench | ||
| from assets.presto_clp.main import presto_clp_bench | ||
| from assets.overhead_test.main import overhead_test_bench | ||
| from assets.gzip.main import gzip_bench | ||
| from src.jsonsync import JsonItem | ||
|
|
@@ -33,31 +33,30 @@ def get_target_from_name(name): | |
|
|
||
|
|
||
| benchmarks = [ # benchmark object, arguments | ||
| (clp_s_bench, {}), | ||
| (clickhouse_native_json_bench, { | ||
| (clp_bench, {}), | ||
| (clickhouse_bench, { | ||
| 'manual_column_names': False, | ||
| 'keys': [], | ||
| 'additional_order_by': [], | ||
| 'timestamp_key': True | ||
| }), | ||
| (clp_presto_bench, { | ||
| (presto_clp_bench, { | ||
| 'dataset_variation': "cleaned_log" | ||
| }), | ||
|
Comment on lines
+36
to
45
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) LGTM! Benchmark list updated correctly. The benchmarks list has been properly updated to use the new class names, maintaining the same configuration parameters. Consider adding a trailing comma after the dictionary on line 44 for consistency: (presto_clp_bench, {
'dataset_variation': "cleaned_log"
- }),
+ }),
🧰 Tools🪛 Ruff (0.12.2)41-41: Trailing comma missing Add trailing comma (COM812) 44-44: Trailing comma missing Add trailing comma (COM812) 🤖 Prompt for AI Agents |
||
| (parquet_bench, {'mode': 'json string'}), | ||
| (parquet_bench, {'mode': 'pairwise arrays'}), | ||
| (presto_parquet_bench, {'mode': 'json string'}), | ||
| (presto_parquet_bench, {'mode': 'pairwise arrays'}), | ||
| (elasticsearch_bench, {}), | ||
| (overhead_test_bench, {}), | ||
| (zstandard_bench, {}), | ||
| (sparksql_bench, {}), | ||
| (gzip_bench, {}), | ||
| ] | ||
|
|
||
| def run(bencher, kwargs, bench_target, attach=False): | ||
| def run(bencher, kwargs, bench_target, attach=False, attach_on_error=False): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add type annotations and consider parameter naming. The function signature has been extended appropriately with the Apply this diff to add type annotations and improve the function signature: -def run(bencher, kwargs, bench_target, attach=False, attach_on_error=False):
+def run(bencher: type, kwargs: dict, bench_target: Path, attach: bool = False, attach_on_error: bool = False) -> None:You'll need to add this import at the top of the file: +from typing import Optional
🧰 Tools🪛 Ruff (0.12.2)55-55: Missing return type annotation for public function Add return type annotation: (ANN201) 55-55: Missing type annotation for function argument (ANN001) 55-55: Missing type annotation for function argument (ANN001) 55-55: Missing type annotation for function argument (ANN001) 55-55: Boolean default positional argument in function definition (FBT002) 55-55: Missing type annotation for function argument (ANN001) 55-55: Boolean default positional argument in function definition (FBT002) 55-55: Missing type annotation for function argument (ANN001) 🤖 Prompt for AI Agents |
||
| dataset_name = 'error when finding dataset name' | ||
| bench = None | ||
| try: | ||
| dataset_name = os.path.basename(bench_target.resolve()).strip() | ||
| # benchmark clp_presto on the cleaned (no spaces) datasets | ||
|
|
||
| print(f'Benchmarking {bencher.__name__} ({kwargs}) on dataset {dataset_name}') | ||
|
|
||
|
|
@@ -70,7 +69,7 @@ def run(bencher, kwargs, bench_target, attach=False): | |
| with open((current_dir / 'exceptions.log').resolve(), 'a') as file: | ||
| file.write(f"{statement}\n") | ||
| print(statement) | ||
| if attach: | ||
| if attach or attach_on_error: | ||
| if bench is not None: | ||
| bench.docker_attach() | ||
| else: | ||
|
|
@@ -82,7 +81,8 @@ def run(bencher, kwargs, bench_target, attach=False): | |
|
|
||
| #if dataset_name != 'mongod': # only use mongod for now | ||
| # continue | ||
| run(bencher, kwargs, bench_target) | ||
| #run(bencher, kwargs, bench_target) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we delete this?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have a change for it but I can't cherry pick it from the initial commit, and it doesn't fit in the scope of this PR. I'll open a separate PR for it when these get merged (or I can put it here). |
||
| run(bencher, kwargs, bench_target, attach_on_error=True) | ||
| #run(bencher, kwargs, bench_target, attach=True) | ||
|
|
||
| #run(sparksql_bench, {}, get_target_from_name('mongod')) | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a reference to
clp_prestoin a comment on line 60.