Feat/first draft - #1
Conversation
Summary of ChangesHello @RianKoja, 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 establishes a robust and automated benchmarking framework for comparing the performance and output consistency of Python dataframe libraries, specifically Pandas, FireDucks, and Polars. It includes tools for generating large-scale synthetic data, executing a broad spectrum of data manipulation tasks, verifying the correctness of results across different frameworks, and producing detailed analytical reports. The entire process is orchestrated via a shell script, ensuring ease of use and reproducibility for evaluating these data processing tools. Highlights
Ignored Files
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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive benchmarking suite for different dataframe libraries. The structure is well-organized with separate scripts for data preparation, benchmarking, comparison, and analysis. The use of a run.sh script to orchestrate everything is great. I've found a few critical issues, especially in the Polars benchmark implementation, which currently benchmarks Pandas instead of Polars. I've also included several suggestions to improve code quality, correctness, and maintainability across the new scripts.
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>
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>
…hmarking into feat/first_draft
chore: first review comments
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive benchmarking suite for comparing pandas, fireducks, and polars. The structure is well-thought-out, with separate scripts for data preparation, benchmarking, result comparison, and analysis. The inclusion of result hashing for correctness verification and detailed performance profiling is excellent. However, I've identified a few critical issues in the benchmark logic for Polars and in the analysis script that need to be addressed. There are also some high-severity typos in the setup script and dependencies that will prevent the suite from running correctly. I've provided detailed comments and suggestions to fix these issues.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…hmarking into feat/first_draft
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive benchmarking suite for dataframe libraries, including data generation, benchmarking scripts for pandas, fireducks, and polars, and analysis tools. The overall structure is well-thought-out, with separate scripts for each stage and a run.sh script to orchestrate the process. The inclusion of result hashing for consistency checks and detailed performance analysis is excellent.
However, there are some critical issues that need to be addressed. The main timing utility in 00_tools.py lacks error handling, which can cause the entire suite to crash on a single operation failure. More importantly, the Polars benchmarks in 03_polars.py are fundamentally flawed: many operations are either timed with costly .to_pandas() conversions or are incorrectly implemented to use pandas logic instead of Polars, invalidating the comparison. I've also included some medium-severity suggestions to improve maintainability by dynamically discovering data and result files instead of using hardcoded lists.
Addressing these issues, especially the critical ones related to benchmarking correctness and robustness, will significantly improve the quality and reliability of this benchmark suite.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive benchmarking suite for pandas, fireducks, and polars. The structure is well-organized into separate scripts for data preparation, benchmarking, comparison, and analysis. The use of pyinstrument for profiling and detailed result comparison is excellent.
My review focuses on improving robustness, consistency, and adherence to best practices. I've identified a critical issue in error handling that would prevent the benchmark suite from running to completion if any single operation fails. I've also included several medium-severity suggestions to improve code quality, such as standardizing argument parsing, using more specific exception types, and fixing a likely typo in the dependencies and CI script. Overall, this is a great foundation for a powerful benchmarking tool.
| def time_operation( | ||
| operation_name: str, | ||
| df_lib: Any, | ||
| func: Callable[..., Any], | ||
| *args: Any, | ||
| **kwargs: Any, | ||
| ) -> Dict[str, Any]: | ||
| """Time a function and return the result and execution info""" | ||
|
|
||
| framework = df_lib.__name__ | ||
| if framework == "fireducks.pandas": | ||
| framework = "fireducks" | ||
|
|
||
| start_time = time.perf_counter() | ||
| result = func(*args, **kwargs) | ||
| # Force evaluation for lazy operations | ||
| if hasattr(result, "compute"): | ||
| result = result.compute() | ||
| elif hasattr(result, "values"): | ||
| _ = result.values # Access values to force computation | ||
| elif isinstance(result, df_lib.DataFrame): | ||
| _ = len(result) # Force evaluation by accessing length | ||
| end_time = time.perf_counter() | ||
| execution_time = end_time - start_time | ||
| success = True | ||
| error_msg = None | ||
|
|
||
| # Save result to parquet file and compute hash | ||
| result_hash = None | ||
| if result is None: | ||
| raise ValueError("Operation returned None, which is not allowed.") | ||
|
|
||
| # Create outputs/results directory if it doesn't exist | ||
| results_dir = "outputs/results" | ||
| os.makedirs(results_dir, exist_ok=True) | ||
|
|
||
| # Save result to parquet file | ||
| output_filename = f"{results_dir}/{operation_name}_{framework}.parquet" | ||
|
|
||
| if hasattr(result, "to_frame"): | ||
| result = result.to_frame(name=operation_name) | ||
|
|
||
| if hasattr(result, "to_parquet"): | ||
| result.to_parquet(output_filename, index=False) | ||
| elif hasattr(result, "write_parquet"): | ||
| result.write_parquet(output_filename) | ||
| else: | ||
| # Handle other types (scalars, arrays, etc.) | ||
| # by converting to DataFrame | ||
| if hasattr(result, "__iter__") and not isinstance(result, (str, bytes)): | ||
| # Iterable but not string | ||
| temp_df = df_lib.DataFrame({"result": list(result)}) | ||
| else: | ||
| # Scalar value | ||
| temp_df = df_lib.DataFrame({"result": [result]}) | ||
| temp_df.to_parquet(output_filename, index=False) | ||
|
|
||
| # Compute hash of the saved file for consistency verification | ||
| with open(output_filename, "rb") as f: | ||
| result_hash = hashlib.sha256(f.read()).hexdigest() | ||
|
|
||
| return { | ||
| "operation": operation_name, | ||
| "framework": framework, | ||
| "execution_time": execution_time, | ||
| "success": success, | ||
| "error": error_msg, | ||
| "timestamp": datetime.now(), | ||
| "result_hash": result_hash, | ||
| } |
There was a problem hiding this comment.
The time_operation function currently does not handle exceptions from the benchmarked function (func). If an operation fails, it will raise an exception and terminate the entire benchmark suite. This contradicts the design of the result dictionary and analysis scripts, which seem prepared to handle failed operations (e.g., via the success and error_msg fields).
Additionally, the file hashing on lines 66-67 reads the entire result file into memory at once, which can be inefficient for very large files. A streaming approach would be more memory-efficient.
I've provided a refactored version of the function that:
- Wraps the operation in a
try...exceptblock to gracefully handle failures and record them. - Uses a streaming approach to compute the file hash, which is more memory-efficient.
def time_operation(
operation_name: str,
df_lib: Any,
func: Callable[..., Any],
*args: Any,
**kwargs: Any,
) -> Dict[str, Any]:
"""Time a function and return the result and execution info"""
framework = df_lib.__name__
if framework == "fireducks.pandas":
framework = "fireducks"
start_time = time.perf_counter()
result_hash = None
success = False
error_msg = None
try:
result = func(*args, **kwargs)
# Force evaluation for lazy operations
if hasattr(result, "compute"):
result = result.compute()
elif hasattr(result, "values"):
_ = result.values # Access values to force computation
elif isinstance(result, df_lib.DataFrame):
_ = len(result) # Force evaluation by accessing length
if result is None:
raise ValueError("Operation returned None, which is not allowed.")
# Create outputs/results directory if it doesn't exist
results_dir = "outputs/results"
os.makedirs(results_dir, exist_ok=True)
# Save result to parquet file
output_filename = f"{results_dir}/{operation_name}_{framework}.parquet"
if hasattr(result, "to_frame"):
result = result.to_frame(name=operation_name)
if hasattr(result, "to_parquet"):
result.to_parquet(output_filename, index=False)
elif hasattr(result, "write_parquet"):
result.write_parquet(output_filename)
else:
# Handle other types (scalars, arrays, etc.)
# by converting to DataFrame
if hasattr(result, "__iter__") and not isinstance(result, (str, bytes)):
# Iterable but not string
temp_df = df_lib.DataFrame({"result": list(result)})
else:
# Scalar value
temp_df = df_lib.DataFrame({"result": [result]})
temp_df.to_parquet(output_filename, index=False)
# Compute hash of the saved file for consistency verification
hasher = hashlib.sha256()
with open(output_filename, "rb") as f:
while chunk := f.read(8192):
hasher.update(chunk)
result_hash = hasher.hexdigest()
success = True
except Exception as e:
error_msg = str(e)
end_time = time.perf_counter()
execution_time = end_time - start_time
return {
"operation": operation_name,
"framework": framework,
"execution_time": execution_time,
"success": success,
"error": error_msg,
"timestamp": datetime.now(),
"result_hash": result_hash,
}Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
No description provided.