Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2259443003:
Capture All Groups in the Field Trial For Testing Studies (Closed)
DescriptionCapture All Groups in the Field Trial For Testing Studies
Instead of selecting the first study in python, this selects the first
study in C++. This allows us to operate on multiple groups if
necessary.
asvitkine@ delegated the review to isherman@.
BUG=637095
TBR=asvitkine
Committed: https://crrev.com/6df952330987ba41ce129b933cf9d15fcdb80d92
Cr-Commit-Position: refs/heads/master@{#413820}
Patch Set 1 #
Total comments: 4
Patch Set 2 : CR Feedback and Change NOTREACHED() to DLOG(ERROR) #
Total comments: 6
Patch Set 3 : CR Feedback #
Messages
Total messages: 40 (25 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Capture All Groups in the Field Trial For Testing Studies BUG= ========== to ========== Capture All Groups in the Field Trial For Testing Studies Instead of selecting the first study in python, this selects the first study in C++. This allows us to operate on multiple groups if necessary. BUG=637095 ==========
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
robliao@chromium.org changed reviewers: + asvitkine@chromium.org
The three CLs were small enough to turn into one CL. Here's part 1 that moves the selection of the first group into C++.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
robliao@chromium.org changed reviewers: + isherman@chromium.org
Redirecting to isherman@ since asvitkine@ is OOO.
LGTM, thanks. By the way, while you're working on this code: We currently have six separate config files, one per target platform. It would be *awesome* to combine these into a single config file, where each study lists its target OSes. It would reduce the amount of boilerplate in our code by a sizeable amount! If you have some spare cycles to implement such a change, I'd very much appreciate it. If you have no spare cycles for yak shaving, I'd totally understand that as well =) https://codereview.chromium.org/2259443003/diff/20001/chrome/common/variation... File chrome/common/variations/fieldtrial_testing_config_schema.json (right): https://codereview.chromium.org/2259443003/diff/20001/chrome/common/variation... chrome/common/variations/fieldtrial_testing_config_schema.json:19: "type_name": "FieldTrialTestingStudyGroup", nit: s/StudyGroup/ExperimentGroup or just Group Technically, the lingo is that a "study" is a collection of groups, and an "experiment" is an individual group. "StudyGroup" is probably technically correct, but I think a bit confusing, as it mixes the "study" and "experiment" concepts somewhat. https://codereview.chromium.org/2259443003/diff/20001/chrome/common/variation... chrome/common/variations/fieldtrial_testing_config_schema.json:27: "type_name": "FieldTrialTestingStudyGroupsParams", (similar comment here)
The file consolidation thing would also require updating our server side logic that looks at those files, so probably out of scope for Robert's change. On Aug 18, 2016 6:09 PM, <isherman@chromium.org> wrote: > LGTM, thanks. > > By the way, while you're working on this code: We currently have six > separate > config files, one per target platform. It would be *awesome* to combine > these > into a single config file, where each study lists its target OSes. It would > reduce the amount of boilerplate in our code by a sizeable amount! If you > have > some spare cycles to implement such a change, I'd very much appreciate it. > If > you have no spare cycles for yak shaving, I'd totally understand that as > well =) > > > https://codereview.chromium.org/2259443003/diff/20001/ > chrome/common/variations/fieldtrial_testing_config_schema.json > File chrome/common/variations/fieldtrial_testing_config_schema.json > (right): > > https://codereview.chromium.org/2259443003/diff/20001/ > chrome/common/variations/fieldtrial_testing_config_schema.json#newcode19 > chrome/common/variations/fieldtrial_testing_config_schema.json:19: > "type_name": "FieldTrialTestingStudyGroup", > nit: s/StudyGroup/ExperimentGroup or just Group > > Technically, the lingo is that a "study" is a collection of groups, and > an "experiment" is an individual group. "StudyGroup" is probably > technically correct, but I think a bit confusing, as it mixes the > "study" and "experiment" concepts somewhat. > > https://codereview.chromium.org/2259443003/diff/20001/ > chrome/common/variations/fieldtrial_testing_config_schema.json#newcode27 > chrome/common/variations/fieldtrial_testing_config_schema.json:27: > "type_name": "FieldTrialTestingStudyGroupsParams", > (similar comment here) > > https://codereview.chromium.org/2259443003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Also changed a NOTREACHED to DLOG(error) to better fit the other exceptional conditions. > By the way, while you're working on this code: We currently have six separate > config files, one per target platform. It would be *awesome* to combine these > into a single config file, where each study lists its target OSes. It would > reduce the amount of boilerplate in our code by a sizeable amount! If you have > some spare cycles to implement such a change, I'd very much appreciate it. If > you have no spare cycles for yak shaving, I'd totally understand that as well =) We might be able to do the preprocessing on the python side and keep the C++ pipeline the same. It would simplify how we handle params across configs. I'll think about it :-). https://codereview.chromium.org/2259443003/diff/20001/chrome/common/variation... File chrome/common/variations/fieldtrial_testing_config_schema.json (right): https://codereview.chromium.org/2259443003/diff/20001/chrome/common/variation... chrome/common/variations/fieldtrial_testing_config_schema.json:19: "type_name": "FieldTrialTestingStudyGroup", On 2016/08/18 22:09:38, Ilya Sherman wrote: > nit: s/StudyGroup/ExperimentGroup or just Group > > Technically, the lingo is that a "study" is a collection of groups, and an > "experiment" is an individual group. "StudyGroup" is probably technically > correct, but I think a bit confusing, as it mixes the "study" and "experiment" > concepts somewhat. Done. Went with just Group. https://codereview.chromium.org/2259443003/diff/20001/chrome/common/variation... chrome/common/variations/fieldtrial_testing_config_schema.json:27: "type_name": "FieldTrialTestingStudyGroupsParams", On 2016/08/18 22:09:38, Ilya Sherman wrote: > (similar comment here) Adjusted to FieldTrialTestingGroupParams
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
robliao@chromium.org changed reviewers: + jwd@chromium.org
jwd: Please review chrome/common/variations/*. Thanks!
On 2016/08/18 23:29:54, robliao wrote: > jwd: Please review chrome/common/variations/*. Thanks! Ping for jwd. Thanks!
Is this intended to solve the problem? or is it a step towards the solution? (just want to make sure I'm not missing something) https://codereview.chromium.org/2259443003/diff/40001/chrome/common/variation... File chrome/common/variations/fieldtrial_testing_config_schema.json (right): https://codereview.chromium.org/2259443003/diff/40001/chrome/common/variation... chrome/common/variations/fieldtrial_testing_config_schema.json:8: "field": "studies", I disagree with Ilya, the terminology is either (study and experiment) or (field trial and group). I'd probably have prefered you use study/experiment, but that would be inconsistent with the testing configs, so I'd like you to change to the field trial/group consistently, and rename this to "trials" or "field_trials". https://codereview.chromium.org/2259443003/diff/40001/chrome/common/variation... File chrome/common/variations/variations_util.cc (right): https://codereview.chromium.org/2259443003/diff/40001/chrome/common/variation... chrome/common/variations/variations_util.cc:30: void AssociateParamsFromGroup(const std::string& study_name, Can you also switch to trial/group terminology in this file too too. https://codereview.chromium.org/2259443003/diff/40001/tools/variations/fieldt... File tools/variations/fieldtrial_to_struct.py (right): https://codereview.chromium.org/2259443003/diff/40001/tools/variations/fieldt... tools/variations/fieldtrial_to_struct.py:69: description='Generates an C++ array of struct from a JSON description.', Sorry to ask you to change things outside of what you're trying to do, but it's bothering me... Is this comment correct? It looks like it just creates a struct, not an array. Also, "Generates a C++..."
Patchset #3 (id:60001) has been deleted
> Is this intended to solve the problem? or is it a step towards the solution? >(just want to make sure I'm not missing something) Yes, the bug contains most of the details. I'm going to be adding forcing flags support to this, which means I'll need all of the groups in C++ land. https://codereview.chromium.org/2259443003/diff/40001/chrome/common/variation... File chrome/common/variations/fieldtrial_testing_config_schema.json (right): https://codereview.chromium.org/2259443003/diff/40001/chrome/common/variation... chrome/common/variations/fieldtrial_testing_config_schema.json:8: "field": "studies", On 2016/08/22 21:54:08, jwd wrote: > I disagree with Ilya, the terminology is either (study and experiment) or (field > trial and group). I'd probably have prefered you use study/experiment, but that > would be inconsistent with the testing configs, so I'd like you to change to the > field trial/group consistently, and rename this to "trials" or "field_trials". Done. Variations uses trial and group too, so it makes more sense to stick with that. https://codereview.chromium.org/2259443003/diff/40001/chrome/common/variation... File chrome/common/variations/variations_util.cc (right): https://codereview.chromium.org/2259443003/diff/40001/chrome/common/variation... chrome/common/variations/variations_util.cc:30: void AssociateParamsFromGroup(const std::string& study_name, On 2016/08/22 21:54:08, jwd wrote: > Can you also switch to trial/group terminology in this file too too. Done. https://codereview.chromium.org/2259443003/diff/40001/tools/variations/fieldt... File tools/variations/fieldtrial_to_struct.py (right): https://codereview.chromium.org/2259443003/diff/40001/tools/variations/fieldt... tools/variations/fieldtrial_to_struct.py:69: description='Generates an C++ array of struct from a JSON description.', On 2016/08/22 21:54:08, jwd wrote: > Sorry to ask you to change things outside of what you're trying to do, but it's > bothering me... > Is this comment correct? It looks like it just creates a struct, not an array. > Also, "Generates a C++..." I've generalized the statement so that it's robust to future changes.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by robliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2259443003/#ps80001 (title: "CR Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Capture All Groups in the Field Trial For Testing Studies Instead of selecting the first study in python, this selects the first study in C++. This allows us to operate on multiple groups if necessary. BUG=637095 ========== to ========== Capture All Groups in the Field Trial For Testing Studies Instead of selecting the first study in python, this selects the first study in C++. This allows us to operate on multiple groups if necessary. asvitkine@ delegated the review to isherman@. BUG=637095 TBR=asvitkine ==========
The CQ bit was checked by robliao@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Capture All Groups in the Field Trial For Testing Studies Instead of selecting the first study in python, this selects the first study in C++. This allows us to operate on multiple groups if necessary. asvitkine@ delegated the review to isherman@. BUG=637095 TBR=asvitkine ========== to ========== Capture All Groups in the Field Trial For Testing Studies Instead of selecting the first study in python, this selects the first study in C++. This allows us to operate on multiple groups if necessary. asvitkine@ delegated the review to isherman@. BUG=637095 TBR=asvitkine ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Capture All Groups in the Field Trial For Testing Studies Instead of selecting the first study in python, this selects the first study in C++. This allows us to operate on multiple groups if necessary. asvitkine@ delegated the review to isherman@. BUG=637095 TBR=asvitkine ========== to ========== Capture All Groups in the Field Trial For Testing Studies Instead of selecting the first study in python, this selects the first study in C++. This allows us to operate on multiple groups if necessary. asvitkine@ delegated the review to isherman@. BUG=637095 TBR=asvitkine Committed: https://crrev.com/6df952330987ba41ce129b933cf9d15fcdb80d92 Cr-Commit-Position: refs/heads/master@{#413820} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/6df952330987ba41ce129b933cf9d15fcdb80d92 Cr-Commit-Position: refs/heads/master@{#413820} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
