Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2400003002:
Remove CL upload logic from Perf Try Job workflow. (Closed)
DescriptionRemove CL upload logic from Perf Try Job workflow.
Currently the perf try job uploads the changes to rietveld before
running git cl try. Since Perf Try job is a wrapper around git cl try,
we think that performing the upload shouldn't be part of Perf Try job.
This wrapper should have the same assumption as "git cl try", the assumption
that the developer is working on an existing, already uploaded CL.
The tool should just make use of the uploaded CL.
Changes to Tool:
a) Display error message if changes are not uploaded.
b) Remove logic to upload changes.
c) Update unittests.
BUG=653665
Committed: https://crrev.com/805fe8562f2b84ab8f9638c82b5ac5d36e6880d7
Cr-Commit-Position: refs/heads/master@{#425691}
Patch Set 1 #
Total comments: 7
Patch Set 2 : . #
Total comments: 4
Patch Set 3 : . #
Messages
Total messages: 25 (12 generated)
Description was changed from ========== Remove CL upload logic from Perf Try Job workflow. Currently the perf try job uploads the changes to rietveld before running git cl try. Since Perf Try job is a wrapper around git cl try, we think that performing the upload shouldn't be part of Perf Try job. This wrapper should have the same assumption as "git cl try", the assumption that the developer is working on an existing, already uploaded CL. The tool should just make use of the uploaded CL. Changes to Tool: a) Display error message if changes are not uploaded. b) Remove logic to upload changes. c) Update unittests. BUG= ========== to ========== Remove CL upload logic from Perf Try Job workflow. Currently the perf try job uploads the changes to rietveld before running git cl try. Since Perf Try job is a wrapper around git cl try, we think that performing the upload shouldn't be part of Perf Try job. This wrapper should have the same assumption as "git cl try", the assumption that the developer is working on an existing, already uploaded CL. The tool should just make use of the uploaded CL. Changes to Tool: a) Display error message if changes are not uploaded. b) Remove logic to upload changes. c) Update unittests. BUG=653665 ==========
prasadv@chromium.org changed reviewers: + machenbach@google.com, nednguyen@google.com, prasadv@chromium.org, sullivan@chromium.org
lgtm lgtm after addressing comment. https://codereview.chromium.org/2400003002/diff/1/tools/perf/core/trybot_comm... File tools/perf/core/trybot_command.py (right): https://codereview.chromium.org/2400003002/diff/1/tools/perf/core/trybot_comm... tools/perf/core/trybot_command.py:452: # Make sure the tree does have local commits. I think we actually don't care about this now as long as _GetChangeList() check below does not fail?
machenbach@chromium.org changed reviewers: + machenbach@chromium.org, tandrii@chromium.org
lgtm for this. Suggesting that you also get ready for the gerrit switch. +tandrii to track the rietveldishness of this tool as blocker for the switch. https://codereview.chromium.org/2400003002/diff/1/tools/perf/core/trybot_comm... File tools/perf/core/trybot_command.py (right): https://codereview.chromium.org/2400003002/diff/1/tools/perf/core/trybot_comm... tools/perf/core/trybot_command.py:508: match = re.search(r'https://codereview.chromium.org/[\d]+', output) This looks like it is rietveld only. Please make sure that the tooling will also work with gerrit as the V8 time will mid-term switch over.
https://codereview.chromium.org/2400003002/diff/1/tools/perf/core/trybot_comm... File tools/perf/core/trybot_command.py (right): https://codereview.chromium.org/2400003002/diff/1/tools/perf/core/trybot_comm... tools/perf/core/trybot_command.py:508: match = re.search(r'https://codereview.chromium.org/[\d]+', output) On 2016/10/07 15:31:12, machenbach (OOO) wrote: > This looks like it is rietveld only. Please make sure that the tooling will also > work with gerrit as the V8 time will mid-term switch over. Please, use --json flag and don't parse human messages that could be changed. $ git cl issue --json output.json
https://codereview.chromium.org/2400003002/diff/1/tools/perf/core/trybot_comm... File tools/perf/core/trybot_command.py (right): https://codereview.chromium.org/2400003002/diff/1/tools/perf/core/trybot_comm... tools/perf/core/trybot_command.py:573: def _RunTryJob(self, bot_platform, arguments, deps_override): this code as is will work with Gerrit too (support for which landing next week).
The CQ bit was checked by prasadv@google.com to run a CQ dry run
https://codereview.chromium.org/2400003002/diff/1/tools/perf/core/trybot_comm... File tools/perf/core/trybot_command.py (right): https://codereview.chromium.org/2400003002/diff/1/tools/perf/core/trybot_comm... tools/perf/core/trybot_command.py:452: # Make sure the tree does have local commits. On 2016/10/07 14:50:07, sullivan wrote: > I think we actually don't care about this now as long as _GetChangeList() check > below does not fail? Done. https://codereview.chromium.org/2400003002/diff/1/tools/perf/core/trybot_comm... tools/perf/core/trybot_command.py:508: match = re.search(r'https://codereview.chromium.org/[\d]+', output) On 2016/10/07 15:41:51, tandrii(chromium) wrote: > On 2016/10/07 15:31:12, machenbach (OOO) wrote: > > This looks like it is rietveld only. Please make sure that the tooling will > also > > work with gerrit as the V8 time will mid-term switch over. > > Please, use --json flag and don't parse human messages that could be changed. > > $ git cl issue --json output.json Done. https://codereview.chromium.org/2400003002/diff/1/tools/perf/core/trybot_comm... tools/perf/core/trybot_command.py:508: match = re.search(r'https://codereview.chromium.org/[\d]+', output) On 2016/10/07 15:31:12, machenbach (slow) wrote: > This looks like it is rietveld only. Please make sure that the tooling will also > work with gerrit as the V8 time will mid-term switch over. Done. Now using git cl issue --json arg to get the issue number and issue url.
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.
https://codereview.chromium.org/2400003002/diff/20001/tools/perf/core/trybot_... File tools/perf/core/trybot_command.py (right): https://codereview.chromium.org/2400003002/diff/20001/tools/perf/core/trybot_... tools/perf/core/trybot_command.py:504: fd, temp_file = tempfile.mkstemp(suffix='.json', prefix='cl') nit: Maybe add a slightly longer and more verbose prefix? I see you delete it below, but if tempfiles are (for whatever reason) not deleted, it would be way easier to reason where they come from. https://codereview.chromium.org/2400003002/diff/20001/tools/perf/core/trybot_... tools/perf/core/trybot_command.py:511: except (IOError, ValueError): Catching this here and setting json_output to None will make json_output.get fail below. And it would be obscure what the original error was. Maybe just don't catch the exceptions or recover from them in a more robust way?
https://codereview.chromium.org/2400003002/diff/20001/tools/perf/core/trybot_... File tools/perf/core/trybot_command.py (right): https://codereview.chromium.org/2400003002/diff/20001/tools/perf/core/trybot_... tools/perf/core/trybot_command.py:504: fd, temp_file = tempfile.mkstemp(suffix='.json', prefix='cl') On 2016/10/08 09:43:29, machenbach (slow) wrote: > nit: Maybe add a slightly longer and more verbose prefix? I see you delete it > below, but if tempfiles are (for whatever reason) not deleted, it would be way > easier to reason where they come from. Done. https://codereview.chromium.org/2400003002/diff/20001/tools/perf/core/trybot_... tools/perf/core/trybot_command.py:511: except (IOError, ValueError): On 2016/10/08 09:43:29, machenbach (slow) wrote: > Catching this here and setting json_output to None will make json_output.get > fail below. And it would be obscure what the original error was. > > Maybe just don't catch the exceptions or recover from them in a more robust way? Done.
lgtm
The CQ bit was checked by sullivan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sullivan@chromium.org Link to the patchset: https://codereview.chromium.org/2400003002/#ps40001 (title: ".")
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 ========== Remove CL upload logic from Perf Try Job workflow. Currently the perf try job uploads the changes to rietveld before running git cl try. Since Perf Try job is a wrapper around git cl try, we think that performing the upload shouldn't be part of Perf Try job. This wrapper should have the same assumption as "git cl try", the assumption that the developer is working on an existing, already uploaded CL. The tool should just make use of the uploaded CL. Changes to Tool: a) Display error message if changes are not uploaded. b) Remove logic to upload changes. c) Update unittests. BUG=653665 ========== to ========== Remove CL upload logic from Perf Try Job workflow. Currently the perf try job uploads the changes to rietveld before running git cl try. Since Perf Try job is a wrapper around git cl try, we think that performing the upload shouldn't be part of Perf Try job. This wrapper should have the same assumption as "git cl try", the assumption that the developer is working on an existing, already uploaded CL. The tool should just make use of the uploaded CL. Changes to Tool: a) Display error message if changes are not uploaded. b) Remove logic to upload changes. c) Update unittests. BUG=653665 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Remove CL upload logic from Perf Try Job workflow. Currently the perf try job uploads the changes to rietveld before running git cl try. Since Perf Try job is a wrapper around git cl try, we think that performing the upload shouldn't be part of Perf Try job. This wrapper should have the same assumption as "git cl try", the assumption that the developer is working on an existing, already uploaded CL. The tool should just make use of the uploaded CL. Changes to Tool: a) Display error message if changes are not uploaded. b) Remove logic to upload changes. c) Update unittests. BUG=653665 ========== to ========== Remove CL upload logic from Perf Try Job workflow. Currently the perf try job uploads the changes to rietveld before running git cl try. Since Perf Try job is a wrapper around git cl try, we think that performing the upload shouldn't be part of Perf Try job. This wrapper should have the same assumption as "git cl try", the assumption that the developer is working on an existing, already uploaded CL. The tool should just make use of the uploaded CL. Changes to Tool: a) Display error message if changes are not uploaded. b) Remove logic to upload changes. c) Update unittests. BUG=653665 Committed: https://crrev.com/805fe8562f2b84ab8f9638c82b5ac5d36e6880d7 Cr-Commit-Position: refs/heads/master@{#425691} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/805fe8562f2b84ab8f9638c82b5ac5d36e6880d7 Cr-Commit-Position: refs/heads/master@{#425691}
Message was sent while issue was closed.
hans@chromium.org changed reviewers: + hans@chromium.org
Message was sent while issue was closed.
Should the docs be updated too? https://chromium.googlesource.com/chromium/src/+/master/docs/speed/perf_trybo... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
