Conversation
ajassani
force-pushed
the
fix/openpyxl_dep
branch
from
92e21dc to
df78a59
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR removes the openpyxl dependency from the package requirements and implements dynamic installation handling. The main purpose is to make openpyxl an optional dependency that is only required when generating Excel files, while also adding CSV output functionality as an alternative.
Key changes include:
- Commenting out openpyxl from setup.py requirements
- Adding dynamic openpyxl installation prompting in the performance report generator
- Implementing CSV output as an alternative to Excel files
Reviewed Changes
Copilot reviewed 3 out of 6 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| setup.py | Removes openpyxl as a required dependency by commenting it out |
| examples/generate_perf_report.py | Adds dynamic openpyxl installation, CSV output functionality, and short kernel analysis features |
| examples/generate_perf_report.md | Adds comprehensive documentation for the performance report generation script |
|
|
||
| def main(): | ||
| def request_install(package_name): |
There was a problem hiding this comment.
Prompting users to install packages via subprocess without validation poses security risks. Consider validating the package_name parameter to prevent arbitrary command execution.
| choice = input(f"Do you want to install '{package_name}' via pip? [y/N]: ").strip().lower() | ||
| if choice == 'y': | ||
| try: | ||
| subprocess.check_call([sys.executable, "-m", "pip", "install", package_name]) |
There was a problem hiding this comment.
Installing packages dynamically via subprocess without validation could allow execution of arbitrary commands if package_name is not properly sanitized.
| # Generate base DataFrames | ||
| df_gpu_timeline = perf_analyzer.get_df_gpu_timeline() | ||
|
|
||
| # TODO: move this to the TreePerfAnalyzer class |
There was a problem hiding this comment.
This TODO comment indicates technical debt where logic should be moved to the TreePerfAnalyzer class for better code organization.
|
|
||
| def get_dfs_short_kernels(perf_analyzer, short_kernel_threshold_us=10, histogram_bins=100, topk=None): | ||
| """ | ||
| TODO: move this to the TreePerfAnalyzer class |
There was a problem hiding this comment.
This TODO comment indicates technical debt where the function should be moved to the TreePerfAnalyzer class for better code organization.
No description provided.