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.

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

Only trigger experiments when a comment starts with /gcbrun #1897

Closed
DonggeLiu opened this issue Sep 13, 2023 · 3 comments · Fixed by #1898
Closed

Only trigger experiments when a comment starts with /gcbrun #1897

DonggeLiu opened this issue Sep 13, 2023 · 3 comments · Fixed by #1898
Assignees

Comments

@DonggeLiu
Copy link
Contributor

DonggeLiu commented Sep 13, 2023

Currently, a GCB experiment will be triggered if our comment mentions /gcbrun.

This is problematic because experiments will be launched unintendedly (e.g., when we quote previous /gcb commands, or even when we mention /gcb in any form).

Root cause

  1. GCB trigger simply checks for /gcb without any constraints.
  2. Our gcbrun_experiment.py iterates all previous comments to find the latest valid /gcb command, which might have already been run.

Proofs

  1. This example quotes an old command and modifies its experiment name. However, it triggers an experiment with the old name, indicating we launch the latest valid experiment command.
  2. This example includes a random /gcb command in the middle of the sentence. It also triggers the same old experiment, confirming the same conclusion.

Propose fix

Change

if not body.startswith(RUN_EXPERIMENT_COMMAND_STR):
continue

to

 if not body.startswith(RUN_EXPERIMENT_COMMAND_STR): 
     return None 

Justification

  1. Based on our setting on Google Cloud, all /gcb commands will immediately trigger an experiment, there is no need to search back in history for an old command.
  2. If we need to run an old command, we can always add a new comment for it.

@jonathanmetzman: Did I miss anything?
If it looks good to you, I will implement the fix proposed above.

@DonggeLiu DonggeLiu self-assigned this Sep 13, 2023
@jonathanmetzman
Copy link
Contributor

you mean [/]gcbrun run right?

@jonathanmetzman
Copy link
Contributor

I think this is a good idea.

DonggeLiu added a commit that referenced this issue Sep 13, 2023
@DonggeLiu DonggeLiu changed the title Only trigger experiments when a comment starts with \gcbrun Only trigger experiments when a comment starts with /gcbrun Sep 13, 2023
@DonggeLiu
Copy link
Contributor Author

you mean [/]gcbrun run right?

Yep, fixed.

jonathanmetzman pushed a commit that referenced this issue Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants