Update report upload logic #972

abohoss · 2025-04-07T17:41:50Z

As mentioned in #971 , I created a upload_report.py file where this python version offers several improvements over the bash script including:

Better Structure:

Object-oriented design with a clear separation of concerns
Methods are modular and reusable

Improved Error Handling:

Comprehensive logging
Proper exception handling
Command execution status checking

Enhanced Features:

Argument parsing with helpful error messages
Better file path handling using Path objects

Better Maintainability:

Logical grouping of related functionality
Easier to extend and modify

DonggeLiu

Thanks @abohoss !
Could you please address the nits below?
I will create a PR to run experiment for your changes : )

DonggeLiu · 2025-04-10T02:06:46Z

report/upload_report.py

+        default_dir = f"{os.getenv('USER')}-{datetime.now().strftime('%Y-%m-%d')}"
+        parser.error(f"This script needs to take gcloud Bucket directory as the second argument. Consider using: {default_dir}")
+    if not args.model:
+        parser.error("This script needs to take LLM as the third argument.")


results_dir (and the other parameters) are positional and therefore automatically required by argparse, the code inside the if not args.results_dir: block will not be executed if results_dir is missing. You can rely on argparse's built-in error handling for missing arguments instead of adding these checks manually.

Additionally, I would prefer placing these arg parsing and handling in an individual function.

DonggeLiu · 2025-04-10T02:09:22Z

report/upload_report.py

+        )
+        self.logger = logging.getLogger(__name__)
+
+    def run_command(self, command: list) -> bool:


nit: Could you please make the functions that are only used in the class private? (e.g., _func).

I made the following function private in the PR : _generate_training_data, _generate_report, _run_command,

Thanks!This helps the linter to report unused functions so that we can remove zombie code.

DonggeLiu · 2025-04-10T02:40:28Z

report/upload_report.py

+        self.benchmark_set = benchmark_set
+        self.model = model
+        self.results_report_dir = Path('results-report')
+        self.bucket_base_path = "gs://oss-fuzz-gcb-experiment-run-logs/Result-reports"


use ' for consistency in python code.

I changed it in all code except this line: self.logger.error(f"Command failed: {' '.join(command)}") ,because using single quotes here can cause a syntax issue because the inner quotes clash with the outer quotes.

How about:

self.logger.error(f'Command failed: {" ".join(command)}')

The main idea is, using ' by default and " only when necessary.

abohoss · 2025-04-11T02:00:48Z

Hi @DonggeLiu ,
Thanks for the insights you provided. Here the PR with the suggested changes: #980

…ss/oss-fuzz-gen into update_report_upload_logic

…d_logic

…ss/oss-fuzz-gen into update_report_upload_logic

DonggeLiu · 2025-04-11T04:04:23Z

Hi @DonggeLiu , Thanks for the insights you provided. Here the PR with the suggested changes: #980

Ah I see you already pushed the new changes here, thanks!
Let me take another look and merge this : )

DonggeLiu

Thanks again, @abohoss!

Some final nits:

DonggeLiu · 2025-04-11T04:11:06Z

report/docker_run.py

  gcs_report_dir = f"{args.sub_dir}/{experiment_name}"

  # Trends report use a similarly named path.
  gcs_trend_report_path = f"{args.sub_dir}/{experiment_name}.json"

  # Generate a report and upload it to GCS
  report_process = subprocess.Popen([
-      "bash", "report/upload_report.sh", local_results_dir, gcs_report_dir,
+      "python3", "report/upload_report.py", local_results_dir, gcs_report_dir,


Sorry that I missed this last time:
Could you please change this python3 to be python_path just like in

oss-fuzz-gen/report/docker_run.py

Line 380 in 9771601

python_path, "run_all_experiments.py", "--benchmarks-directory",

This is to ensure the correct python is used in the docker container.

DonggeLiu · 2025-04-11T04:12:37Z

report/upload_report.py

+        self.logger.info('Final report uploaded.')
+
+
+def parse_args():


Add return type.

def parse_args() -> argparse.Namespace:

DonggeLiu · 2025-04-11T04:14:33Z

report/upload_report.py

+        )
+        self.logger = logging.getLogger(__name__)
+
+    def run_command(self, command: list) -> bool:


Thanks!This helps the linter to report unused functions so that we can remove zombie code.

DonggeLiu · 2025-04-11T04:15:40Z

report/upload_report.py

+        self.benchmark_set = benchmark_set
+        self.model = model
+        self.results_report_dir = Path('results-report')
+        self.bucket_base_path = "gs://oss-fuzz-gcb-experiment-run-logs/Result-reports"


How about:

self.logger.error(f'Command failed: {" ".join(command)}')

The main idea is, using ' by default and " only when necessary.

…ss/oss-fuzz-gen into update_report_upload_logic

abohoss · 2025-04-11T19:23:02Z

Hi @DonggeLiu ,
I addressed the changes, Thanks for your time !

DonggeLiu · 2025-04-14T05:28:10Z

Thanks for addressing this so promptly!
I will merge this to the experiment PR (#977) so that we can run your code before merging into main.

As mentioned in #971 , I created a `upload_report.py` file where this python version offers several improvements over the bash script including: 1. Better Structure: - Object-oriented design with a clear separation of concerns - Methods are modular and reusable 2. Improved Error Handling: - Comprehensive logging - Proper exception handling - Command execution status checking 3. Enhanced Features: - Argument parsing with helpful error messages - Better file path handling using Path objects 4. Better Maintainability: - Logical grouping of related functionality - Easier to extend and modify --------- Co-authored-by: Dongge Liu <[email protected]>

DonggeLiu · 2025-04-14T05:34:14Z

Ops The linter CI reported some issues.
Given this PR has been merged and the branch is deleted, could you please address them in new branch/PR based on exp-972?

This script will help you identify the error locally pushing : )
https://github.com/google/oss-fuzz-gen/blob/main/USAGE.md#contribution-process

abohoss · 2025-04-14T19:36:41Z

Thanks for addressing this so promptly! I will merge this to the experiment PR (#977) so that we can run your code before merging into main.

You're welcome!

abohoss · 2025-04-14T19:39:02Z

Ops The linter CI reported some issues. Given this PR has been merged and the branch is deleted, could you please address them in new branch/PR based on exp-972?

This script will help you identify the error locally pushing : ) https://github.com/google/oss-fuzz-gen/blob/main/USAGE.md#contribution-process

Hi @DonggeLiu!
I pushed the linting changes to #988

abohoss and others added 3 commits March 31, 2025 23:22

Add success property to RunResult class to address TODO

11c5f96

updated report upload logic to use python file instead of bash script

aff618d

Merge branch 'main' into update_report_upload_logic

9f0b5f5

DonggeLiu reviewed Apr 10, 2025

View reviewed changes

DonggeLiu changed the base branch from main to exp-972 April 10, 2025 02:46

abohoss and others added 8 commits April 11, 2025 04:07

Merge branch 'exp-972' into update_report_upload_logic

8417b5b

worked on changes provided by maintainer(Dongge)

a6f83c5

Merge branch 'update_report_upload_logic' of https://github.com/aboho…

3f47cac

…ss/oss-fuzz-gen into update_report_upload_logic

Merge remote-tracking branch 'upstream/main' into update_report_uploa…

2f5b1f9

…d_logic

Merge branch 'google:main' into update_report_upload_logic

cf21624

Merge branch 'update_report_upload_logic' of https://github.com/aboho…

d4e6a9e

…ss/oss-fuzz-gen into update_report_upload_logic

this commit to make compare branches work

1a2f5ad

this commit to make compare branches work 2.0

9871656

DonggeLiu mentioned this pull request Apr 11, 2025

Update report upload logic #980

Closed

Merge branch 'exp-972' into update_report_upload_logic

9771601

DonggeLiu reviewed Apr 11, 2025

View reviewed changes

DonggeLiu marked this pull request as draft April 11, 2025 05:40

abohoss added 2 commits April 11, 2025 21:18

address provided nits

beb2b4a

Merge branch 'update_report_upload_logic' of https://github.com/aboho…

bd6ee88

…ss/oss-fuzz-gen into update_report_upload_logic

abohoss marked this pull request as ready for review April 11, 2025 19:27

DonggeLiu merged commit aff2fc8 into google:exp-972 Apr 14, 2025
1 of 2 checks passed

DonggeLiu mentioned this pull request Apr 14, 2025

@abohoss: Rewrite upload_report in python. #977

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update report upload logic #972

Update report upload logic #972

abohoss commented Apr 7, 2025 •

edited

Loading

DonggeLiu left a comment

DonggeLiu Apr 10, 2025

DonggeLiu Apr 10, 2025

DonggeLiu Apr 10, 2025

abohoss Apr 11, 2025

DonggeLiu Apr 11, 2025

DonggeLiu Apr 10, 2025

abohoss Apr 11, 2025

DonggeLiu Apr 11, 2025

abohoss commented Apr 11, 2025 •

edited

Loading

DonggeLiu commented Apr 11, 2025

DonggeLiu left a comment

DonggeLiu Apr 11, 2025

DonggeLiu Apr 11, 2025

DonggeLiu Apr 11, 2025

DonggeLiu Apr 11, 2025

abohoss commented Apr 11, 2025

DonggeLiu commented Apr 14, 2025 •

edited

Loading

DonggeLiu commented Apr 14, 2025 •

edited

Loading

abohoss commented Apr 14, 2025

abohoss commented Apr 14, 2025

Update report upload logic #972

Update report upload logic #972

Conversation

abohoss commented Apr 7, 2025 • edited Loading

DonggeLiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abohoss commented Apr 11, 2025 • edited Loading

DonggeLiu commented Apr 11, 2025

DonggeLiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abohoss commented Apr 11, 2025

DonggeLiu commented Apr 14, 2025 • edited Loading

DonggeLiu commented Apr 14, 2025 • edited Loading

abohoss commented Apr 14, 2025

abohoss commented Apr 14, 2025

abohoss commented Apr 7, 2025 •

edited

Loading

abohoss commented Apr 11, 2025 •

edited

Loading

DonggeLiu commented Apr 14, 2025 •

edited

Loading

DonggeLiu commented Apr 14, 2025 •

edited

Loading