Skip to content
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.

Sign up for GitHub

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

Jump to bottom

@Snehil-Shah: refactor to allow arbitrary args to experiments #885

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

@Snehil-Shah: refactor to allow arbitrary args to experiments #885

wants to merge 4 commits into from
+324 −235

Conversation

DonggeLiu
Copy link
Collaborator

Running experiments for #854, an amazing work contributed by @Snehil-Shah.

@DonggeLiu
Create temp
Loading
Loading status checks…
572ffd1
@DonggeLiu DonggeLiu mentioned this pull request Mar 17, 2025
Merged
@Snehil-Shah
exp: refactor to allow arbitrary args to experiments (#854)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
Loading
Loading status checks…
cd01b73 
- ### What can you do now?
You can pass arguments directly to `run_all_experiments.py` using
additional arguments after `--`.
	```bash
	request_pr_exp.py -p=45 -n=exp -- --model=vertex_ai_gemini-1-5
	```
Here, `--model=vertex_ai_gemini-1-5` will be directly passed to
`run_all_experiments.py` along with the usual arguments.

- ### Couple of pointers:

- Earlier `REDIRECT_OUTS` was being passed as an environment variable. I
changed it to be passed directly as an argument for the sake of
consistency. Lemme know if it's not desired behavior.
- The boolean arguments in `docker_run.py` are just strings that were
passed earlier for the bash script, and not booleans. This made YAML
substitution easier rather than supporting `store_true` arguments.

---------

Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
@DonggeLiu
Copy link
Collaborator Author

/gcbrun exp -n ss -ag

@DonggeLiu
Copy link
Collaborator Author

/gcbrun exp -n ss -- -ag -l vertex_ai_gemini-2-chat

@DonggeLiu
Delete temp
Loading
Loading status checks…
fb638b9
@DonggeLiu
Copy link
Collaborator Author

Hi @Snehil-Shah ,
I presume the error is because of this line?

Step #3: INFO:root:Command: ['-n', 'ss', '--', '-ag', '-l', 'vertex_ai_gemini-2-chat', '-p', '885'].
Step #3: usage: ci_trial_build.py [-h] [-c CLUSTER] [-l LOCATION] [-t GKE_TEMPLATE] -p
Step #3:                          PR_ID -n NAME_SUFFIX [-b BENCHMARK_SET] [-m LLM]
Step #3:                          [-ll LLM_LOCATIONS] [-d DELAY] [-to FUZZING_TIMEOUT]
Step #3:                          [-rc REQUEST_CPUS] [-rm REQUEST_MEMORY] [-i]
Step #3:                          [-ns NUM_SAMPLES] [-nf LLM_FIX_LIMIT]
Step #3:                          [-vt VARY_TEMPERATURE] [-mr MAX_ROUND] [-ag] [-lg]
Step #3:                          [-rd]
Step #3: ci_trial_build.py: error: the following arguments are required: -p/--pr-id
Finished Step #3
ERROR
ERROR: build step 3 "us-central1-docker.pkg.dev/oss-fuzz-base/oss-fuzz-gen/ci" failed: step exited with non-zero status: 2

@Snehil-Shah
Copy link
Contributor

@DonggeLiu Ah right, the added PR id argument is clubbed with the optional arguments as we are appending it towards the end. Will change ci_build.py to add it before our command.

@Snehil-Shah Snehil-Shah mentioned this pull request Mar 18, 2025
Merged
@Snehil-Shah
fix: correct args order in CI trial build script (#893)
Loading
Loading status checks…
cb1800e 
For #885

Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
@DonggeLiu
Copy link
Collaborator Author

/gcbrun exp -n ss -- -ag -l vertex_ai_gemini-2-chat

@DonggeLiu DonggeLiu requested a review from erfanio March 20, 2025 04:05
@DonggeLiu
Copy link
Collaborator Author

I think this is ready to merge. @erfanio @oliverchang Do you have any suggestions?

There are some trivial improvements, but we can do them later:

  1. LLM model name passed to run_all_experiments.py is not reflected on report, resulting int wrong LLM names on report pages.
  2. Some args are no longer necessary given we can directly pass them to run_all_experiments.py
  3. Instead, we may be able to come up with some short-cut flags for PR experiments (e.g., default values for PR exps, name exp based on time, etc.).

All these suggest we can shift the weight from 'passing all kinds of args' to 'more intelligently design the logics to process run_all_experiments.py args'.

@Snehil-Shah Please focus on your proposal, that is the main factor for application evalaution.
Please also list your PRs in your proposal so that we can take them into considerations.

@DonggeLiu DonggeLiu requested a review from oliverchang March 20, 2025 04:18
@Snehil-Shah
Copy link
Contributor

LLM model name passed to run_all_experiments.py is not reflected on report, resulting int wrong LLM names on report pages.

@DonggeLiu we can pass the additional arguments to the report.trends_report.upload_summary script as well to fix this?

@DonggeLiu
Copy link
Collaborator Author

LLM model name passed to run_all_experiments.py is not reflected on report, resulting int wrong LLM names on report pages.

@DonggeLiu we can pass the additional arguments to the report.trends_report.upload_summary script as well to fix this?

I was referring to this:

oss-fuzz-gen/report/templates/base.html

Lines 104 to 107 in 5a8c71e

<body>
LLM: {{ model }}
{% block content %}{% endblock %}
</body>

Which appears on all pages.

But yes, we should also pass it to trends_report.
Feel free to leave this in a later PR and focus on your report now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erfanio erfanio

@oliverchang oliverchang

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@DonggeLiu @Snehil-Shah