Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2309473002:
Extend Perf Try Job support to other chromium repos. (Closed)
|
Created:
4 years, 3 months ago by prasadv Modified:
3 years, 4 months ago CC:
chromium-reviews, telemetry-reviews_chromium.org, Vadim Sh., tandrii(chromium), agable Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExtend Perf Try Job support to other chromium repos.
1. Perf try jobs now supports DEPS repos such as v8, angle, skia along with src.
2. Added new two arguments --repo_path and --deps_revision.
--repo_path: This is the path to the repo directory. (default: chromium src path)
if given the perf try job will create patch and run try job from the given repo
--deps_revision: DEPS repo revision on which patch is applied. If this is not given
then perf try job will get the base revision from upstream on which
the current changes are created.
Design doc: http://goto/zvbzub
BUG=636505
Committed: https://crrev.com/9f919c116a98ab5e4c782d753f5be6ed4e2f64fc
Cr-Commit-Position: refs/heads/master@{#420933}
Patch Set 1 #
Total comments: 19
Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : . #
Total comments: 4
Patch Set 5 : . #
Total comments: 12
Patch Set 6 : Rebasing and fix unittest #
Messages
Total messages: 44 (18 generated)
Description was changed from
==========
Extend Perf Try Job support to other chromium repos.
1. Perf try jobs now supports DEPS repos such as v8, angle, skia along with
src.
2. Added new two arguments --repo_path and --deps_revision.
--repo_path: This is the path to the repo directory. (default: chromium src
path)
if given the perf try job will create patch and run try job
from the given repo
--deps_revision: DEPS repo revision on which patch is applied. If this is
not given
then perf try job will get the base revision from upstream
on which
the current changes are created.
BUG=636505
==========
to
==========
Extend Perf Try Job support to other chromium repos.
1. Perf try jobs now supports DEPS repos such as v8, angle, skia along with
src.
2. Added new two arguments --repo_path and --deps_revision.
--repo_path: This is the path to the repo directory. (default: chromium src
path)
if given the perf try job will create patch and run try job
from the given repo
--deps_revision: DEPS repo revision on which patch is applied. If this is
not given
then perf try job will get the base revision from upstream
on which
the current changes are created.
Design doc: http://goto/zvbzub
BUG=636505
==========
As suggested earlier, I split the CL into 2 parts. This CL contains only changes related to supporting other chromium repos. Other part is related to git cl try refactor is already landed. Thank you,
https://codereview.chromium.org/2309473002/diff/1/tools/perf/core/trybot_comm... File tools/perf/core/trybot_command.py (right): https://codereview.chromium.org/2309473002/diff/1/tools/perf/core/trybot_comm... tools/perf/core/trybot_command.py:84: } Any reason not to add catapult here as well? https://codereview.chromium.org/2309473002/diff/1/tools/perf/core/trybot_comm... tools/perf/core/trybot_command.py:316: metavar='<repo path>') This is supposed to be specified relative to cwd? For example, if you're in src/ directory, you would say 'v8' or 'third_party/skia'? And if you're in v8 directory, it would be '.'? That's kind of confusing, especially since IIRC telemetry supports running from a wide variety of cwds. I think it would be clearer for users to specify 'v8', 'skia', 'angle' always, with None for chromium. Especially since you already have to store the lookup info in REPO_INFO_MAP anyway. https://codereview.chromium.org/2309473002/diff/1/tools/perf/core/trybot_comm... tools/perf/core/trybot_command.py:319: help='specify DEPS revision to be used by Chromium.\n', Would be great to clarify what this argument does a little in the help text: * When would you want to specify a DEPS revision? * I assume DEPS revision changes the value in DEPS for the repo specified in --repo_path? * Does this apply to with patch, without patch, or both? https://codereview.chromium.org/2309473002/diff/1/tools/perf/core/trybot_comm... tools/perf/core/trybot_command.py:338: raise TrybotError('Repository path "%s" does not exists, please check ' s/does not exists/does not exist/ https://codereview.chromium.org/2309473002/diff/1/tools/perf/core/trybot_comm... tools/perf/core/trybot_command.py:341: os.chdir(repo_path) Why the need for os.chdir here? https://codereview.chromium.org/2309473002/diff/1/tools/perf/core/trybot_comm... tools/perf/core/trybot_command.py:465: 'Failed to get upstream branch name.') Isn't this an infinite loop? https://codereview.chromium.org/2309473002/diff/1/tools/perf/core/trybot_comm... tools/perf/core/trybot_command.py:498: raise TrybotError('Unsupported repository %s' % repo_name) Here the error would be clearer if users specified repo_name instead of repo_path.
https://codereview.chromium.org/2309473002/diff/1/tools/perf/core/trybot_comm... File tools/perf/core/trybot_command.py (right): https://codereview.chromium.org/2309473002/diff/1/tools/perf/core/trybot_comm... tools/perf/core/trybot_command.py:84: } On 2016/09/08 03:22:59, sullivan wrote: > Any reason not to add catapult here as well? I added support for catapult too. https://codereview.chromium.org/2309473002/diff/1/tools/perf/core/trybot_comm... tools/perf/core/trybot_command.py:316: metavar='<repo path>') On 2016/09/08 03:22:59, sullivan wrote: > This is supposed to be specified relative to cwd? For example, if you're in src/ > directory, you would say 'v8' or 'third_party/skia'? And if you're in v8 > directory, it would be '.'? No, this is the complete path to the directory where the patch exist. > > That's kind of confusing, especially since IIRC telemetry supports running from > a wide variety of cwds. I think it would be clearer for users to specify 'v8', > 'skia', 'angle' always, with None for chromium. Especially since you already > have to store the lookup info in REPO_INFO_MAP anyway. As discussed with v8 about normal workflow for v8 development, they create separate directory (not in src/) for normal development. Therefore we don't want to assume the patches are in repos relative to src/ directory. https://codereview.chromium.org/2309473002/diff/1/tools/perf/core/trybot_comm... tools/perf/core/trybot_command.py:319: help='specify DEPS revision to be used by Chromium.\n', On 2016/09/08 03:22:59, sullivan wrote: > Would be great to clarify what this argument does a little in the help text: > > * When would you want to specify a DEPS revision? This is used when we want to modify DEPS entry in Chromium to a certain pushed revision. If this is not specified then the tool will determine the DEPS revision on top of which the changes are made. > * I assume DEPS revision changes the value in DEPS for the repo specified in > --repo_path? Yes, this changes the DEPS revision for the repo in TOT chromium. > * Does this apply to with patch, without patch, or both? This is applied while process both (with and without patch). https://codereview.chromium.org/2309473002/diff/1/tools/perf/core/trybot_comm... tools/perf/core/trybot_command.py:338: raise TrybotError('Repository path "%s" does not exists, please check ' On 2016/09/08 03:22:59, sullivan wrote: > s/does not exists/does not exist/ Done. https://codereview.chromium.org/2309473002/diff/1/tools/perf/core/trybot_comm... tools/perf/core/trybot_command.py:341: os.chdir(repo_path) On 2016/09/08 03:22:59, sullivan wrote: > Why the need for os.chdir here? As mentioned in the previous comments, repo_path is the path to the directory where the patch exists. For dependency repo's workflow it is not always true that they do development under src/ directory. https://codereview.chromium.org/2309473002/diff/1/tools/perf/core/trybot_comm... tools/perf/core/trybot_command.py:465: 'Failed to get upstream branch name.') On 2016/09/08 03:22:59, sullivan wrote: > Isn't this an infinite loop? I don't think so, because every time we enter the loop branch_name is set to the upstream of current branch. This loop will break and raise TrybotError exception when a branch upstream is not to the branch properly.
https://codereview.chromium.org/2309473002/diff/1/tools/perf/core/trybot_comm... File tools/perf/core/trybot_command.py (right): https://codereview.chromium.org/2309473002/diff/1/tools/perf/core/trybot_comm... tools/perf/core/trybot_command.py:316: metavar='<repo path>') On 2016/09/08 19:03:32, prasadv wrote: > On 2016/09/08 03:22:59, sullivan wrote: > > This is supposed to be specified relative to cwd? For example, if you're in > src/ > > directory, you would say 'v8' or 'third_party/skia'? And if you're in v8 > > directory, it would be '.'? > > No, this is the complete path to the directory where the patch exist. > > > > That's kind of confusing, especially since IIRC telemetry supports running > from > > a wide variety of cwds. I think it would be clearer for users to specify 'v8', > > 'skia', 'angle' always, with None for chromium. Especially since you already > > have to store the lookup info in REPO_INFO_MAP anyway. > > As discussed with v8 about normal workflow for v8 development, they create > separate directory (not in src/) for normal development. Therefore we don't want > to assume the patches are in repos relative to src/ directory. > I think the documentation needs to be clearer, especially since you've got different user groups with different assumptions: v8 team would be running this script with a directory not relative to src/, but with the way the catapult documentation talks about developing in Chrome, catapult developers likely wouldn't know you could run this with a path that isn't third_party/catapult. https://codereview.chromium.org/2309473002/diff/1/tools/perf/core/trybot_comm... tools/perf/core/trybot_command.py:341: os.chdir(repo_path) On 2016/09/08 19:03:32, prasadv wrote: > On 2016/09/08 03:22:59, sullivan wrote: > > Why the need for os.chdir here? > > As mentioned in the previous comments, repo_path is the path to the directory > where the patch exists. For dependency repo's workflow it is not always true > that they do development under src/ directory. So it looks like _AttemptTryJob requires you to be in repo_path and also takes repo_path as an argument. Shouldn't the try, os.chdir, finally all be in _AttemptTryJob? https://codereview.chromium.org/2309473002/diff/1/tools/perf/core/trybot_comm... tools/perf/core/trybot_command.py:465: 'Failed to get upstream branch name.') On 2016/09/08 19:03:33, prasadv wrote: > On 2016/09/08 03:22:59, sullivan wrote: > > Isn't this an infinite loop? > > I don't think so, because every time we enter the loop branch_name is set to the > upstream of current branch. > This loop will break and raise TrybotError exception when a branch upstream is > not to the branch properly. I think adding a high-level comment to explain what the loop does would help here.
https://codereview.chromium.org/2309473002/diff/1/tools/perf/core/trybot_comm... File tools/perf/core/trybot_command.py (right): https://codereview.chromium.org/2309473002/diff/1/tools/perf/core/trybot_comm... tools/perf/core/trybot_command.py:316: metavar='<repo path>') On 2016/09/08 20:43:57, sullivan wrote: > On 2016/09/08 19:03:32, prasadv wrote: > > On 2016/09/08 03:22:59, sullivan wrote: > > > This is supposed to be specified relative to cwd? For example, if you're in > > src/ > > > directory, you would say 'v8' or 'third_party/skia'? And if you're in v8 > > > directory, it would be '.'? > > > > No, this is the complete path to the directory where the patch exist. > > > > > > That's kind of confusing, especially since IIRC telemetry supports running > > from > > > a wide variety of cwds. I think it would be clearer for users to specify > 'v8', > > > 'skia', 'angle' always, with None for chromium. Especially since you already > > > have to store the lookup info in REPO_INFO_MAP anyway. > > > > As discussed with v8 about normal workflow for v8 development, they create > > separate directory (not in src/) for normal development. Therefore we don't > want > > to assume the patches are in repos relative to src/ directory. > > > > I think the documentation needs to be clearer, especially since you've got > different user groups with different assumptions: v8 team would be running this > script with a directory not relative to src/, but with the way the catapult > documentation talks about developing in Chrome, catapult developers likely > wouldn't know you could run this with a path that isn't third_party/catapult. Please note that the path info in REPO_INFO_MAP is not the actual path to the patches. They are are related to path to the repo in the # DEPS file, These values are used create the DEPS patch along with patch that is being tried. I also added detailed explanation about --repo_path in command help. https://codereview.chromium.org/2309473002/diff/1/tools/perf/core/trybot_comm... tools/perf/core/trybot_command.py:341: os.chdir(repo_path) On 2016/09/08 20:43:56, sullivan wrote: > On 2016/09/08 19:03:32, prasadv wrote: > > On 2016/09/08 03:22:59, sullivan wrote: > > > Why the need for os.chdir here? > > > > As mentioned in the previous comments, repo_path is the path to the directory > > where the patch exists. For dependency repo's workflow it is not always true > > that they do development under src/ directory. > > So it looks like _AttemptTryJob requires you to be in repo_path and also takes > repo_path as an argument. Shouldn't the try, os.chdir, finally all be in > _AttemptTryJob? Done. https://codereview.chromium.org/2309473002/diff/1/tools/perf/core/trybot_comm... tools/perf/core/trybot_command.py:465: 'Failed to get upstream branch name.') On 2016/09/08 20:43:57, sullivan wrote: > On 2016/09/08 19:03:33, prasadv wrote: > > On 2016/09/08 03:22:59, sullivan wrote: > > > Isn't this an infinite loop? > > > > I don't think so, because every time we enter the loop branch_name is set to > the > > upstream of current branch. > > This loop will break and raise TrybotError exception when a branch upstream is > > not to the branch properly. > > I think adding a high-level comment to explain what the loop does would help > here. Done.
lgtm https://codereview.chromium.org/2309473002/diff/60001/tools/perf/core/trybot_... File tools/perf/core/trybot_command_unittest.py (right): https://codereview.chromium.org/2309473002/diff/60001/tools/perf/core/trybot_... tools/perf/core/trybot_command_unittest.py:318: repo_path=trybot_command.CHROMIUM_SRC_PATH, deps_revision=None) These 3 lines are copy-pasted through the whole file. Maybe we should make a _DEFAULT_OPTIONS global and use it instead? https://codereview.chromium.org/2309473002/diff/60001/tools/perf/core/trybot_... tools/perf/core/trybot_command_unittest.py:532: 'Could not try CL for linux', Just curious if it's possible to give a more specific error here to the user about DEPS override failing?
https://codereview.chromium.org/2309473002/diff/60001/tools/perf/core/trybot_... File tools/perf/core/trybot_command_unittest.py (right): https://codereview.chromium.org/2309473002/diff/60001/tools/perf/core/trybot_... tools/perf/core/trybot_command_unittest.py:318: repo_path=trybot_command.CHROMIUM_SRC_PATH, deps_revision=None) On 2016/09/09 02:42:46, sullivan wrote: > These 3 lines are copy-pasted through the whole file. Maybe we should make a > _DEFAULT_OPTIONS global and use it instead? Done. https://codereview.chromium.org/2309473002/diff/60001/tools/perf/core/trybot_... tools/perf/core/trybot_command_unittest.py:532: 'Could not try CL for linux', On 2016/09/09 02:42:46, sullivan wrote: > Just curious if it's possible to give a more specific error here to the user > about DEPS override failing? Done.
prasadv@chromium.org changed reviewers: + fmeawad@chromium.org
Hi Macheal, Could you please take a look at this CL.
Ping! PTAL
Sorry for not looking into this earlier. Despite implementation details below, my main usability concern is that this tool uploads CLs. This doesn't really fit to V8 developer workflows, e.g. 1. Make WIP CL, 2. let some folks look at it, run normal trybots, 3. run also perf trybots on the existing CL. I assume the uploading stems from earlier times of this script when a local cfg file was created? Since this isn't the case anymore, maybe free this script of this additional concern? https://codereview.chromium.org/2309473002/diff/80001/tools/perf/core/trybot_... File tools/perf/core/trybot_command.py (right): https://codereview.chromium.org/2309473002/diff/80001/tools/perf/core/trybot_... tools/perf/core/trybot_command.py:69: # that is being tried. I think we should try to not introduce yet another of these mappings. Future stakeholders will wonder, "Why doesn't it work for my project?". This should be generic (or at least configured in one place) and I left comments below, how to do without REPO_INFO_MAP. https://codereview.chromium.org/2309473002/diff/80001/tools/perf/core/trybot_... tools/perf/core/trybot_command.py:531: if not options.deps_revision: Why is this feature limited to non-src repos? I find this feature orthogonal to allowing deps tryjobs. Against which revision are src CLs tried? I'd be fine with trying against a default like HEAD or DEPS revision of the repo, still allowing manual override with deps_revision. A feature that calculates the locally used upstream revision is neat, but I don't understand why it is tied to this CL and not already present for src tryjobs. https://codereview.chromium.org/2309473002/diff/80001/tools/perf/core/trybot_... tools/perf/core/trybot_command.py:533: branch_name, repo_info.get('url')) You could get the remote repo url from the local repo itself, not needing a static mapping. https://codereview.chromium.org/2309473002/diff/80001/tools/perf/core/trybot_... tools/perf/core/trybot_command.py:534: deps_override = {repo_info.get('src'): options.deps_revision} You shouldn't need to specify the name of the deps here if the project has set up the patch_project setting in codereview.settings file. The mapping of patch_project name to deps name already exists for many repos, e.g.: https://cs.chromium.org/chromium/tools/depot_tools/recipe_modules/gclient/con... Bot_update automatically updates the HEAD of the listed repositories when checking out chromium and with project set to one of the listed projects. It should be easy to override HEAD on the recipe side to something else. https://codereview.chromium.org/2309473002/diff/80001/tools/perf/core/trybot_... tools/perf/core/trybot_command.py:538: rietveld_url = self._UploadPatchToRietveld(repo_name, options) This code here has nothing to do with the arguments calculation above. Maybe change the order of steps and group things that are clearly dependent on each other? Or do you want to make sure to fail early? Furthermore, why is this script at all concerned with uploading to rietveld? IMO this script should rather deal with already uploaded CLs.
Thank you Michael for you comments. In order to run the try job we need to upload the CL, otherwise the bot will not know what changes to patch. We could have used buildbucket, but I don't think sending patch with buildbucket is supported yet. That is the reason we are uploading the patch. https://codereview.chromium.org/2309473002/diff/80001/tools/perf/core/trybot_... File tools/perf/core/trybot_command.py (right): https://codereview.chromium.org/2309473002/diff/80001/tools/perf/core/trybot_... tools/perf/core/trybot_command.py:69: # that is being tried. On 2016/09/20 10:18:20, machenbach (slow) wrote: > I think we should try to not introduce yet another of these mappings. Future > stakeholders will wonder, "Why doesn't it work for my project?". This should be > generic (or at least configured in one place) and I left comments below, how to > do without REPO_INFO_MAP. As mentioned in the comments, these mappings are necessary in order to validate the repos correctly. 'src': Its value is mapped to the repo path in DEPS file. In order to apply the DEPS revision on the patch we use the DEPS path + DEPS revision as a property to buildbucket. Without this mapping buildbucket will not know be apple to apply DEPS revision. 'url': This is the remote URL of the repo which is used to validate whether the repo we are processing is supported by tryjob. Mostly this is used to avoid running try jobs for unsupported repos. https://codereview.chromium.org/2309473002/diff/80001/tools/perf/core/trybot_... tools/perf/core/trybot_command.py:533: branch_name, repo_info.get('url')) On 2016/09/20 10:18:20, machenbach (slow) wrote: > You could get the remote repo url from the local repo itself, not needing a > static mapping. Agreed, but we want to validate the remote URL to check if it is supported by perf try job. https://codereview.chromium.org/2309473002/diff/80001/tools/perf/core/trybot_... tools/perf/core/trybot_command.py:534: deps_override = {repo_info.get('src'): options.deps_revision} On 2016/09/20 10:18:20, machenbach (slow) wrote: > You shouldn't need to specify the name of the deps here if the project has set > up the patch_project setting in codereview.settings file. The mapping of > patch_project name to deps name already exists for many repos, e.g.: > https://cs.chromium.org/chromium/tools/depot_tools/recipe_modules/gclient/con... > > Bot_update automatically updates the HEAD of the listed repositories when > checking out chromium and with project set to one of the listed projects. It > should be easy to override HEAD on the recipe side to something else. Actually running the tests and building the binary happens on two separate bots. When processing the try job the try bot post a build request to the builder along with the patch. And the builder expects the dep_override along with name of the deps. https://codereview.chromium.org/2309473002/diff/80001/tools/perf/core/trybot_... tools/perf/core/trybot_command.py:538: rietveld_url = self._UploadPatchToRietveld(repo_name, options) On 2016/09/20 10:18:20, machenbach (slow) wrote: > This code here has nothing to do with the arguments calculation above. Maybe > change the order of steps and group things that are clearly dependent on each > other? Or do you want to make sure to fail early? > > Furthermore, why is this script at all concerned with uploading to rietveld? IMO > this script should rather deal with already uploaded CLs. Perf try job uses git cl try to run the try job on the patch by uploading it to the rietveld. This will work for both the case i.e., already uploaded CL and a not uploaded CL(the try job will upload it).
On 2016/09/20 19:12:01, prasadv wrote: > Thank you Michael for you comments. > > In order to run the try job we need to upload the CL, otherwise the bot will not > know what changes to patch. > We could have used buildbucket, but I don't think sending patch with buildbucket > is supported yet. > That is the reason we are uploading the patch. Replying only to this part. I'm aware ot that a CL has to be uploaded in order to use tryjobs. But normal try workflow is the same - you do a sequence of commands: git cl upload git cl try Normal "git cl try" uses buildbucket behind the scenes unless you actively opt out. This is also the only way to pass parameters with the -p option AFAIK. I'm fine with what this script does as long as it supports already uploaded CLs. The rest is rather implementation detail. In general I'd like to push the following things where we can: 1. Keep things as generic as possible, avoid hardcoded extra configurations that can be derived. 2. Limit redundancy where configurations are needed. 3. Move as much of the logic into the try recipes as possible. A local script should only be a simple wrapper and not contain the meat.
On 2016/09/20 20:40:41, machenbach (slow) wrote: > On 2016/09/20 19:12:01, prasadv wrote: > > Thank you Michael for you comments. > > > > In order to run the try job we need to upload the CL, otherwise the bot will > not > > know what changes to patch. > > We could have used buildbucket, but I don't think sending patch with > buildbucket > > is supported yet. > > That is the reason we are uploading the patch. > > Replying only to this part. > > I'm aware ot that a CL has to be uploaded in order to use tryjobs. But normal > try workflow is the same - you do a sequence of commands: > git cl upload > git cl try > > Normal "git cl try" uses buildbucket behind the scenes unless you actively opt > out. This is also the only way to pass parameters with the -p option AFAIK. > Sorry I should have mentioned this before, our initial plan was to use buildbucket API and get rid of uploading patches to rietveld. Last time when I checked with nodir@ buildbucket doesn't support sending patches. Tt needs a reference to the location where the patch exists. And that is why we thought of uploading CL for try jobs. Please correct me if I' missing anything here.
Adding vadimsh, tandrii, and agable from infra team. Looks like there's some confusion over what's possible here: * Currently, this creates a new branch and a new CL to make a perf tryjob * Is it possible to just use buildbucket without creating a CL? * Is it possible to check if a CL exists and use that instead of a new one?
On 2016/09/20 22:27:34, sullivan wrote: > Adding vadimsh, tandrii, and agable from infra team. Looks like there's some > confusion over what's possible here: > > * Currently, this creates a new branch and a new CL to make a perf tryjob > * Is it possible to just use buildbucket without creating a CL? AFAIK a CL must exist so that bot_update can get the patch from somewhere. > * Is it possible to check if a CL exists and use that instead of a new one? Let me try to clarify more. I was never thinking of an approach that works without uploaded CLs (maybe possible at some point, but completely orthogonal to this CL). I just think that "performing the upload" should not be part of this tryjob-wrapper. This wrapper should have the same assumption as "git cl try", which it wraps - the assumption that the developer is working on an existing, already uploaded CL. The tool should just make use of the uploaded CL. If the dev doesn't upload the latest changes, his/her problem. If you want the tool to also upload in case there's no CL yet, fine, as long as it works also for existing CLs and e.g. not creates a new patchset each time somebody calls this script.
On 2016/09/21 08:47:57, machenbach (slow) wrote: > On 2016/09/20 22:27:34, sullivan wrote: > > Adding vadimsh, tandrii, and agable from infra team. Looks like there's some > > confusion over what's possible here: > > > > * Currently, this creates a new branch and a new CL to make a perf tryjob > > * Is it possible to just use buildbucket without creating a CL? > > AFAIK a CL must exist so that bot_update can get the patch from somewhere. > > > * Is it possible to check if a CL exists and use that instead of a new one? > > Let me try to clarify more. I was never thinking of an approach that works > without uploaded CLs (maybe possible at some point, but completely orthogonal to > this CL). > > I just think that "performing the upload" should not be part of this > tryjob-wrapper. This wrapper should have the same assumption as "git cl try", > which it wraps - the assumption that the developer is working on an existing, > already uploaded CL. The tool should just make use of the uploaded CL. If the > dev doesn't upload the latest changes, his/her problem. I agree with this. We actually already check that the client is in a state to upload a CL, instead of going through all that work and giving errors related to it, we could check if there is a CL uploaded, and give an error if not. I think that + an email to telemetry-announce@chromium.org for the functionality change would be strictly better for users. Prasad, sorry to keep breaking this change up, but do you think you could switch to that workflow in a separate CL? > If you want the tool to also upload in case there's no CL yet, fine, as long as > it works also for existing CLs and e.g. not creates a new patchset each time > somebody calls this script.
On 2016/09/21 13:41:55, sullivan wrote: > On 2016/09/21 08:47:57, machenbach (slow) wrote: > > On 2016/09/20 22:27:34, sullivan wrote: > > > Adding vadimsh, tandrii, and agable from infra team. Looks like there's some > > > confusion over what's possible here: > > > > > > * Currently, this creates a new branch and a new CL to make a perf tryjob > > > * Is it possible to just use buildbucket without creating a CL? > > > > AFAIK a CL must exist so that bot_update can get the patch from somewhere. > > > > > * Is it possible to check if a CL exists and use that instead of a new one? > > > > Let me try to clarify more. I was never thinking of an approach that works > > without uploaded CLs (maybe possible at some point, but completely orthogonal > to > > this CL). > > > > I just think that "performing the upload" should not be part of this > > tryjob-wrapper. This wrapper should have the same assumption as "git cl try", > > which it wraps - the assumption that the developer is working on an existing, > > already uploaded CL. The tool should just make use of the uploaded CL. If the > > dev doesn't upload the latest changes, his/her problem. > > I agree with this. We actually already check that the client is in a state to > upload a CL, instead of going through all that work and giving errors related to > it, we could check if there is a CL uploaded, and give an error if not. I think > that + an email to mailto:telemetry-announce@chromium.org for the functionality change > would be strictly better for users. > > Prasad, sorry to keep breaking this change up, but do you think you could switch > to that workflow in a separate CL? > > > If you want the tool to also upload in case there's no CL yet, fine, as long > as > > it works also for existing CLs and e.g. not creates a new patchset each time > > somebody calls this script. I think workflow changes should be a part of separate CL. Since this change is purely related to supporting try job DEPS. I don't want to mix both the changes in a same CL. If everyone is OK with the current state of this CL then I'll land this CL and upload a separate patch for Perf tryjob workflow change.
On 2016/09/21 17:41:10, prasadv wrote: > On 2016/09/21 13:41:55, sullivan wrote: > > On 2016/09/21 08:47:57, machenbach (slow) wrote: > > > On 2016/09/20 22:27:34, sullivan wrote: > > > > Adding vadimsh, tandrii, and agable from infra team. Looks like there's > some > > > > confusion over what's possible here: > > > > > > > > * Currently, this creates a new branch and a new CL to make a perf tryjob > > > > * Is it possible to just use buildbucket without creating a CL? > > > > > > AFAIK a CL must exist so that bot_update can get the patch from somewhere. > > > > > > > * Is it possible to check if a CL exists and use that instead of a new > one? > > > > > > Let me try to clarify more. I was never thinking of an approach that works > > > without uploaded CLs (maybe possible at some point, but completely > orthogonal > > to > > > this CL). > > > > > > I just think that "performing the upload" should not be part of this > > > tryjob-wrapper. This wrapper should have the same assumption as "git cl > try", > > > which it wraps - the assumption that the developer is working on an > existing, > > > already uploaded CL. The tool should just make use of the uploaded CL. If > the > > > dev doesn't upload the latest changes, his/her problem. > > > > I agree with this. We actually already check that the client is in a state to > > upload a CL, instead of going through all that work and giving errors related > to > > it, we could check if there is a CL uploaded, and give an error if not. I > think > > that + an email to mailto:telemetry-announce@chromium.org for the > functionality change > > would be strictly better for users. > > > > Prasad, sorry to keep breaking this change up, but do you think you could > switch > > to that workflow in a separate CL? > > > > > If you want the tool to also upload in case there's no CL yet, fine, as long > > as > > > it works also for existing CLs and e.g. not creates a new patchset each time > > > somebody calls this script. > > I think workflow changes should be a part of separate CL. Since this change is > purely related to supporting try job DEPS. I don't want to mix both the changes > in a same CL. > If everyone is OK with the current state of this CL then I'll land this CL and > upload a separate patch for Perf tryjob workflow change. It seems better to do the workflow change first, but if machenbach@ is okay with landing this and then improving the workflow, I'm okay as well.
fine - lgtm from v8 perspective one comment below might have slipped through the cracks? https://codereview.chromium.org/2309473002/diff/80001/tools/perf/core/trybot_... File tools/perf/core/trybot_command.py (right): https://codereview.chromium.org/2309473002/diff/80001/tools/perf/core/trybot_... tools/perf/core/trybot_command.py:531: if not options.deps_revision: On 2016/09/20 10:18:20, machenbach (slow) wrote: > Why is this feature limited to non-src repos? I find this feature orthogonal to > allowing deps tryjobs. Against which revision are src CLs tried? > > I'd be fine with trying against a default like HEAD or DEPS revision of the > repo, still allowing manual override with deps_revision. > > A feature that calculates the locally used upstream revision is neat, but I > don't understand why it is tied to this CL and not already present for src > tryjobs. ok - could you comment also on this remark?
https://codereview.chromium.org/2309473002/diff/80001/tools/perf/core/trybot_... File tools/perf/core/trybot_command.py (right): https://codereview.chromium.org/2309473002/diff/80001/tools/perf/core/trybot_... tools/perf/core/trybot_command.py:531: if not options.deps_revision: On 2016/09/20 10:18:20, machenbach (slow) wrote: > Why is this feature limited to non-src repos? I find this feature orthogonal to > allowing deps tryjobs. Against which revision are src CLs tried? > > I'd be fine with trying against a default like HEAD or DEPS revision of the > repo, still allowing manual override with deps_revision. > > A feature that calculates the locally used upstream revision is neat, but I > don't understand why it is tied to this CL and not already present for src > tryjobs. src CLs are always tried against TOT chromium revisions, therefore we don't override DEPS revisions for src. Moreover we want to override deps revision only for DEPs repo try jobs because the TOT chromium revision might not be pinned to the locally used upstream revision for the CL.
still lgtm. treat my comments more like food for thoughts for the future... nothing blocking. https://codereview.chromium.org/2309473002/diff/80001/tools/perf/core/trybot_... File tools/perf/core/trybot_command.py (right): https://codereview.chromium.org/2309473002/diff/80001/tools/perf/core/trybot_... tools/perf/core/trybot_command.py:531: if not options.deps_revision: On 2016/09/22 17:50:31, prasadv wrote: > On 2016/09/20 10:18:20, machenbach (slow) wrote: > > Why is this feature limited to non-src repos? I find this feature orthogonal > to > > allowing deps tryjobs. Against which revision are src CLs tried? > > > > I'd be fine with trying against a default like HEAD or DEPS revision of the > > repo, still allowing manual override with deps_revision. > > > > A feature that calculates the locally used upstream revision is neat, but I > > don't understand why it is tied to this CL and not already present for src > > tryjobs. > > src CLs are always tried against TOT chromium revisions, therefore we don't > override DEPS revisions for src. > Moreover we want to override deps revision only for DEPs repo try jobs because > the TOT chromium revision might not be pinned to the locally used upstream > revision for the CL. > Why not always run against ToT of the deps repo? Any CL under developent is usually supposed to work with ToT of the respective repo. If devs want something else they can manually override. And it's never guaranteed anyway that a locally used version of a deps repo will fit to Chromium ToT. One reason I've heard from developers, why they'd want to perf test against an older revision and not ToT, is to keep their results comparable (from patch set to patch set). But that use case would apply to Chromium as the main project as well.
The CQ bit was checked by prasadv@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/2309473002/#ps80001 (title: ".")
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by prasadv@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: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by prasadv@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.
The CQ bit was checked by prasadv@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from machenbach@chromium.org, sullivan@chromium.org Link to the patchset: https://codereview.chromium.org/2309473002/#ps100001 (title: "Rebasing and fix unittest")
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
==========
Extend Perf Try Job support to other chromium repos.
1. Perf try jobs now supports DEPS repos such as v8, angle, skia along with
src.
2. Added new two arguments --repo_path and --deps_revision.
--repo_path: This is the path to the repo directory. (default: chromium src
path)
if given the perf try job will create patch and run try job
from the given repo
--deps_revision: DEPS repo revision on which patch is applied. If this is
not given
then perf try job will get the base revision from upstream
on which
the current changes are created.
Design doc: http://goto/zvbzub
BUG=636505
==========
to
==========
Extend Perf Try Job support to other chromium repos.
1. Perf try jobs now supports DEPS repos such as v8, angle, skia along with
src.
2. Added new two arguments --repo_path and --deps_revision.
--repo_path: This is the path to the repo directory. (default: chromium src
path)
if given the perf try job will create patch and run try job
from the given repo
--deps_revision: DEPS repo revision on which patch is applied. If this is
not given
then perf try job will get the base revision from upstream
on which
the current changes are created.
Design doc: http://goto/zvbzub
BUG=636505
==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from
==========
Extend Perf Try Job support to other chromium repos.
1. Perf try jobs now supports DEPS repos such as v8, angle, skia along with
src.
2. Added new two arguments --repo_path and --deps_revision.
--repo_path: This is the path to the repo directory. (default: chromium src
path)
if given the perf try job will create patch and run try job
from the given repo
--deps_revision: DEPS repo revision on which patch is applied. If this is
not given
then perf try job will get the base revision from upstream
on which
the current changes are created.
Design doc: http://goto/zvbzub
BUG=636505
==========
to
==========
Extend Perf Try Job support to other chromium repos.
1. Perf try jobs now supports DEPS repos such as v8, angle, skia along with
src.
2. Added new two arguments --repo_path and --deps_revision.
--repo_path: This is the path to the repo directory. (default: chromium src
path)
if given the perf try job will create patch and run try job
from the given repo
--deps_revision: DEPS repo revision on which patch is applied. If this is
not given
then perf try job will get the base revision from upstream
on which
the current changes are created.
Design doc: http://goto/zvbzub
BUG=636505
Committed: https://crrev.com/9f919c116a98ab5e4c782d753f5be6ed4e2f64fc
Cr-Commit-Position: refs/heads/master@{#420933}
==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/9f919c116a98ab5e4c782d753f5be6ed4e2f64fc Cr-Commit-Position: refs/heads/master@{#420933} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
