Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2573913002:
Add devil dependencies config for performance tests.
|
Created:
4 years ago by djordje.golubovic Modified:
3 years, 11 months ago CC:
chromium-reviews, telemetry-reviews_chromium.org, petar.jovanovic, gordana.cmiljanovic_imgtec.com Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd devil dependencies config for performance tests.
Lets performance tests use local binaries for md5sum_device and
forwarder_device when running on MIPS32, as they aren't stored in gs.
Also adds local push_apps_to_background_apk to binary_dependencies
for MIPS32.
BUG=669849
Patch Set 1 #
Total comments: 6
Patch Set 2 : Inject devil_chromium module into telemetry binary_manager #Patch Set 3 : Call devil_chromium.Initialize(...) from chromium_config module #
Total comments: 3
Messages
Total messages: 44 (10 generated)
djordje.golubovic@imgtec.com changed reviewers: + nednguyen@google.com
Proposing fix for https://bugs.chromium.org/p/chromium/issues/detail?id=669849 Please take a look.
Description was changed from ========== Add devil dependencies config for performance tests. Lets performance tests use local binaries for md5sum_device and forwarder_device when running on MIPS32, as they aren't stored in gs. Also adds local push_apps_to_background_apk to binary_dependencies for MIPS32. BUG=669849 ========== to ========== Add devil dependencies config for performance tests. Lets performance tests use local binaries for md5sum_device and forwarder_device when running on MIPS32, as they aren't stored in gs. Also adds local push_apps_to_background_apk to binary_dependencies for MIPS32. BUG=669849 ==========
nednguyen@google.com changed reviewers: + eyaich@chromium.org
On 2016/12/13 18:33:11, djordje.golubovic wrote: > Proposing fix for https://bugs.chromium.org/p/chromium/issues/detail?id=669849 > > Please take a look. I defer reviewing this to Emily
eyaich@chromium.org changed reviewers: + aiolos@chromium.org
I am not super familiar with devil_env. Kari do you have any thoughts around this CL?
aiolos@chromium.org changed reviewers: + jbudorick@chromium.org
On 2016/12/16 15:29:22, eyaich1 wrote: > I am not super familiar with devil_env. > > Kari do you have any thoughts around this CL? The bug is that Telemetry and Devil don't support the android_mips platform. This isn't how to fix that bug for two reasons: 1) Telemetry shouldn't be forcing libraries we depend on to unknowingly support platforms since that is super fragile. 2) Devil is an internal implementation detail of Telemetry and should not be referenced outside of telemetry/internal. 3) It might not actually enable running telemetry tests on android_mips because devil isn't our only android dependency. I'll add notes in the linked bug on how to actually solve this.
On 2016/12/16 18:20:27, aiolos wrote: > On 2016/12/16 15:29:22, eyaich1 wrote: > > I am not super familiar with devil_env. > > > > Kari do you have any thoughts around this CL? > > The bug is that Telemetry and Devil don't support the android_mips platform. > This isn't how to fix that bug for two reasons: > > 1) Telemetry shouldn't be forcing libraries we depend on to unknowingly support > platforms since that is super fragile. > 2) Devil is an internal implementation detail of Telemetry and should not be > referenced outside of telemetry/internal. > 3) It might not actually enable running telemetry tests on android_mips because > devil isn't our only android dependency. > > I'll add notes in the linked bug on how to actually solve this. Actually, I take that back. It looks like devil does support mips, but only in the chromium repo. This should get fixed by a change inside telemetry's dependency_manager.
MIPS isn't currently supported in stock devil because we haven't needed it. Adding the MIPS binaries to the devil config in catapult wouldn't be too difficult. https://codereview.chromium.org/2573913002/diff/1/tools/perf/chrome_telemetry... File tools/perf/chrome_telemetry_build/chromium_config.py (right): https://codereview.chromium.org/2573913002/diff/1/tools/perf/chrome_telemetry... tools/perf/chrome_telemetry_build/chromium_config.py:57: devil_env.config.Initialize(config_files=[DEVIL_CONFIG_PATH]) The right way to handle this would be to call devil_chromium.Initialize(output_directory=...), but to do that, we'd need to get the chromium output directory down here.
https://codereview.chromium.org/2573913002/diff/1/tools/perf/chrome_telemetry... File tools/perf/chrome_telemetry_build/chromium_config.py (right): https://codereview.chromium.org/2573913002/diff/1/tools/perf/chrome_telemetry... tools/perf/chrome_telemetry_build/chromium_config.py:57: devil_env.config.Initialize(config_files=[DEVIL_CONFIG_PATH]) This should instead be done in telemetry's binary manager, which knows about the Chromium src dir (and therefore the output dir) if we're in a Chromium checkout.
On 2016/12/16 18:39:23, aiolos wrote: > https://codereview.chromium.org/2573913002/diff/1/tools/perf/chrome_telemetry... > File tools/perf/chrome_telemetry_build/chromium_config.py (right): > > https://codereview.chromium.org/2573913002/diff/1/tools/perf/chrome_telemetry... > tools/perf/chrome_telemetry_build/chromium_config.py:57: > devil_env.config.Initialize(config_files=[DEVIL_CONFIG_PATH]) > This should instead be done in telemetry's binary manager, which knows about the > Chromium src dir (and therefore the output dir) if we're in a Chromium checkout. sgtm
https://codereview.chromium.org/2573913002/diff/1/tools/perf/chrome_telemetry... File tools/perf/chrome_telemetry_build/chromium_config.py (right): https://codereview.chromium.org/2573913002/diff/1/tools/perf/chrome_telemetry... tools/perf/chrome_telemetry_build/chromium_config.py:57: devil_env.config.Initialize(config_files=[DEVIL_CONFIG_PATH]) On 2016/12/16 18:39:22, aiolos wrote: > This should instead be done in telemetry's binary manager, which knows about the > Chromium src dir (and therefore the output dir) if we're in a Chromium checkout. Thanks for your inputs. I just need a bit of clarification - does this mean devil_chromium.Initialize(output_directory=...) should be called from binary_manager in third_party/catapult? If so, devil_chromium module must (as far as I know) be imported from Chromium src dir (another checkout), which sounds suspicious to me. Or, devil_chromium.Initialize(...) could be called here (same checkout), and use telemetry.core.util.GetBuildDirectories() to get Chromium out dir (this module already knows where telemetry is).
https://codereview.chromium.org/2573913002/diff/1/tools/perf/chrome_telemetry... File tools/perf/chrome_telemetry_build/chromium_config.py (right): https://codereview.chromium.org/2573913002/diff/1/tools/perf/chrome_telemetry... tools/perf/chrome_telemetry_build/chromium_config.py:57: devil_env.config.Initialize(config_files=[DEVIL_CONFIG_PATH]) On 2016/12/20 15:16:03, djordje.golubovic wrote: > On 2016/12/16 18:39:22, aiolos wrote: > > This should instead be done in telemetry's binary manager, which knows about > the > > Chromium src dir (and therefore the output dir) if we're in a Chromium > checkout. > > Thanks for your inputs. > It seems that they're not entirely compatible, unfortunately. :( > I just need a bit of clarification - does this mean > devil_chromium.Initialize(output_directory=...) should be called from > binary_manager in third_party/catapult? If so, devil_chromium module must (as > far as I know) be imported from Chromium src dir (another checkout), which > sounds suspicious to me. > > Or, devil_chromium.Initialize(...) could be called here (same checkout), and use > telemetry.core.util.GetBuildDirectories() to get Chromium out dir (this module > already knows where telemetry is). I'll defer to aiolos for what the best course of action is, although my relatively uninformed guess would that it'd be the latter option.
On 2016/12/20 16:55:44, jbudorick wrote: > https://codereview.chromium.org/2573913002/diff/1/tools/perf/chrome_telemetry... > File tools/perf/chrome_telemetry_build/chromium_config.py (right): > > https://codereview.chromium.org/2573913002/diff/1/tools/perf/chrome_telemetry... > tools/perf/chrome_telemetry_build/chromium_config.py:57: > devil_env.config.Initialize(config_files=[DEVIL_CONFIG_PATH]) > On 2016/12/20 15:16:03, djordje.golubovic wrote: > > On 2016/12/16 18:39:22, aiolos wrote: > > > This should instead be done in telemetry's binary manager, which knows about > > the > > > Chromium src dir (and therefore the output dir) if we're in a Chromium > > checkout. > > > > Thanks for your inputs. > > > > It seems that they're not entirely compatible, unfortunately. :( > > > I just need a bit of clarification - does this mean > > devil_chromium.Initialize(output_directory=...) should be called from > > binary_manager in third_party/catapult? If so, devil_chromium module must (as > > far as I know) be imported from Chromium src dir (another checkout), which > > sounds suspicious to me. Telemetry runs both inside and outside of a Chromium checkout, and has multiple places in it's internal code with Chromium-specific logic that only gets used in the checkout. Example: https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... > > > > Or, devil_chromium.Initialize(...) could be called here (same checkout), and > use > > telemetry.core.util.GetBuildDirectories() to get Chromium out dir (this module > > already knows where telemetry is). > > I'll defer to aiolos for what the best course of action is, although my > relatively uninformed guess would that it'd be the latter option. John: does devil check whether the output directory exists? And does it handle multiple directories (ie, checking Release, Debug, etc)? Does initializing with devil_chromium work with the fact that telemetry makes calls to catapult's devil? I strongly don't want initialize to be called here like this, because it would silently skip the initialization in internal telemetry code. Which would be a serious pain to debug if it breaks something in the future. The chromium config was specifically made as a way to pipe Chromium specific information into Telemetry's dependency manager. Options on proceeding with just supporting android mips in a Chromium checkout: 1) use chromium config to pass the devil_chromium module into the dependency manager, and call initialize on the passed in module if it exists, and on the currently used devil module if it doesn't. 2) call initialize here, and pipe in to the dependency manager the fact that initialize has already been called. Instead of calling initialize again, the dependency manager should log a warning that devil was previously initialized to help with future debugging. I have a preference for the 1st option. Note that telemetry.core.util.GetBuildDirectories returns all of the build directories we might be interested in, not just one. Current initialization location: https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... Possible gotchas: initializing devil this way means that telemetry will (probably) support android_mips inside a Chromium checkout, but not outside of one. This means we will not be running any tests on android_mips on the catapult waterfall. It also doesn't give access to running on a reference build, which is useful for catching performance regressions. And it still might not work in general cases due to Telemetry needing other platform specific dependencies (like memtrack_helper), or less likely because we don't correctly prefetch dependencies for android mips in dependency_manager.FetchBinaryDepdencies. I'm still in favor of adding full support - including cloud storage dependencies - in both devil and telemetry if android mips is a useful platform to be running telemetry tests on. Just supporting through a Chromium checkout seems really fragile.
On 2016/12/20 19:13:32, aiolos wrote: > On 2016/12/20 16:55:44, jbudorick wrote: > > > https://codereview.chromium.org/2573913002/diff/1/tools/perf/chrome_telemetry... > > File tools/perf/chrome_telemetry_build/chromium_config.py (right): > > > > > https://codereview.chromium.org/2573913002/diff/1/tools/perf/chrome_telemetry... > > tools/perf/chrome_telemetry_build/chromium_config.py:57: > > devil_env.config.Initialize(config_files=[DEVIL_CONFIG_PATH]) > > On 2016/12/20 15:16:03, djordje.golubovic wrote: > > > On 2016/12/16 18:39:22, aiolos wrote: > > > > This should instead be done in telemetry's binary manager, which knows > about > > > the > > > > Chromium src dir (and therefore the output dir) if we're in a Chromium > > > checkout. > > > > > > Thanks for your inputs. > > > > > > > It seems that they're not entirely compatible, unfortunately. :( > > > > > I just need a bit of clarification - does this mean > > > devil_chromium.Initialize(output_directory=...) should be called from > > > binary_manager in third_party/catapult? If so, devil_chromium module must > (as > > > far as I know) be imported from Chromium src dir (another checkout), which > > > sounds suspicious to me. > > Telemetry runs both inside and outside of a Chromium checkout, and has multiple > places in it's internal code with Chromium-specific logic that only gets used in > the checkout. Example: > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... > > > > > > > Or, devil_chromium.Initialize(...) could be called here (same checkout), and > > use > > > telemetry.core.util.GetBuildDirectories() to get Chromium out dir (this > module > > > already knows where telemetry is). > > > > I'll defer to aiolos for what the best course of action is, although my > > relatively uninformed guess would that it'd be the latter option. > > John: does devil check whether the output directory exists? And does it handle Not at initialization time. We use FetchPath, though, so if a configured binary does not exist, we'll fall back to the version in GCS. > multiple directories (ie, checking Release, Debug, etc)? Does initializing with I'm not sure; this is definitely not an intended use case for devil_chromium, though. Does telemetry configured multiple output directories? > devil_chromium work with the fact that telemetry makes calls to catapult's > devil? Yeah, this would be fine. devil_env is a per-process global that can only be initialized once. devil_chromium initializes it with various binaries available to chromium (both checked-in and in the output directory) in addition to the GCS versions. > > I strongly don't want initialize to be called here like this, because it would > silently skip the initialization in internal telemetry code. Which would be a > serious pain to debug if it breaks something in the future. > > The chromium config was specifically made as a way to pipe Chromium specific > information into Telemetry's dependency manager. Options on proceeding with just This was the same motivation behind devil_chromium. > supporting android mips in a Chromium checkout: 1) use chromium config to pass > the devil_chromium module into the dependency manager, and call initialize on > the passed in module if it exists, and on the currently used devil module if it > doesn't. 2) call initialize here, and pipe in to the dependency manager the fact > that initialize has already been called. Instead of calling initialize again, > the dependency manager should log a warning that devil was previously > initialized to help with future debugging. I have a preference for the 1st > option. devil_env handles possible subsequent calls to Initialize, though it doesn't log anything: https://codesearch.chromium.org/chromium/src/third_party/catapult/devil/devil... We could also add something like IsInitialized to devil_env._Environment s.t. the initialization status doesn't have to be piped through -- telemetry could merely check whether devil_env.config has been initialized and then either initialize it or log a warning as appropriate. > > Note that telemetry.core.util.GetBuildDirectories returns all of the build > directories we might be interested in, not just one. I'm still a little confused as to this setup & how it works. > > Current initialization location: > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... > > Possible gotchas: initializing devil this way means that telemetry will > (probably) support android_mips inside a Chromium checkout, but not outside of > one. This means we will not be running any tests on android_mips on the catapult > waterfall. It also doesn't give access to running on a reference build, which is > useful for catching performance regressions. And it still might not work in > general cases due to Telemetry needing other platform specific dependencies > (like memtrack_helper), or less likely because we don't correctly prefetch > dependencies for android mips in dependency_manager.FetchBinaryDepdencies. I'm > still in favor of adding full support - including cloud storage dependencies - > in both devil and telemetry if android mips is a useful platform to be running > telemetry tests on. Just supporting through a Chromium checkout seems really > fragile. This would mean that telemetry would work with any locally built architecture (i.e., x86 and x86_64, too), not just mips. Without reference builds, GCS binaries, or automated testing, I'd be hesitant to call it official support. I don't think we want to add any more platforms while we're manually supporting the GCS binaries. Once we get something automated (crbug.com/618019), perhaps we could consider it.
On 2016/12/20 19:34:48, jbudorick wrote: > On 2016/12/20 19:13:32, aiolos wrote: > > On 2016/12/20 16:55:44, jbudorick wrote: > > > > > > https://codereview.chromium.org/2573913002/diff/1/tools/perf/chrome_telemetry... > > > File tools/perf/chrome_telemetry_build/chromium_config.py (right): > > > > > > > > > https://codereview.chromium.org/2573913002/diff/1/tools/perf/chrome_telemetry... > > > tools/perf/chrome_telemetry_build/chromium_config.py:57: > > > devil_env.config.Initialize(config_files=[DEVIL_CONFIG_PATH]) > > > On 2016/12/20 15:16:03, djordje.golubovic wrote: > > > > On 2016/12/16 18:39:22, aiolos wrote: > > > > > This should instead be done in telemetry's binary manager, which knows > > about > > > > the > > > > > Chromium src dir (and therefore the output dir) if we're in a Chromium > > > > checkout. > > > > > > > > Thanks for your inputs. > > > > > > > > > > It seems that they're not entirely compatible, unfortunately. :( > > > > > > > I just need a bit of clarification - does this mean > > > > devil_chromium.Initialize(output_directory=...) should be called from > > > > binary_manager in third_party/catapult? If so, devil_chromium module must > > (as > > > > far as I know) be imported from Chromium src dir (another checkout), which > > > > sounds suspicious to me. > > > > Telemetry runs both inside and outside of a Chromium checkout, and has > multiple > > places in it's internal code with Chromium-specific logic that only gets used > in > > the checkout. Example: > > > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... > > > > > > > > > > Or, devil_chromium.Initialize(...) could be called here (same checkout), > and > > > use > > > > telemetry.core.util.GetBuildDirectories() to get Chromium out dir (this > > module > > > > already knows where telemetry is). > > > > > > I'll defer to aiolos for what the best course of action is, although my > > > relatively uninformed guess would that it'd be the latter option. > > > > John: does devil check whether the output directory exists? And does it handle > > Not at initialization time. We use FetchPath, though, so if a configured binary > does not exist, we'll fall back to the version in GCS. > > > multiple directories (ie, checking Release, Debug, etc)? Does initializing > with > > I'm not sure; this is definitely not an intended use case for devil_chromium, > though. Does telemetry configured multiple output directories? > > > devil_chromium work with the fact that telemetry makes calls to catapult's > > devil? > > Yeah, this would be fine. devil_env is a per-process global that can only be > initialized once. devil_chromium initializes it with various binaries available > to chromium (both checked-in and in the output directory) in addition to the GCS > versions. > > > > > I strongly don't want initialize to be called here like this, because it would > > silently skip the initialization in internal telemetry code. Which would be a > > serious pain to debug if it breaks something in the future. > > > > The chromium config was specifically made as a way to pipe Chromium specific > > information into Telemetry's dependency manager. Options on proceeding with > just > > This was the same motivation behind devil_chromium. > > > supporting android mips in a Chromium checkout: 1) use chromium config to pass > > the devil_chromium module into the dependency manager, and call initialize on > > the passed in module if it exists, and on the currently used devil module if > it > > doesn't. 2) call initialize here, and pipe in to the dependency manager the > fact > > that initialize has already been called. Instead of calling initialize again, > > the dependency manager should log a warning that devil was previously > > initialized to help with future debugging. I have a preference for the 1st > > option. > > devil_env handles possible subsequent calls to Initialize, though it doesn't log > anything: > https://codesearch.chromium.org/chromium/src/third_party/catapult/devil/devil... > > We could also add something like IsInitialized to devil_env._Environment s.t. > the initialization status doesn't have to be piped through -- telemetry could > merely check whether devil_env.config has been initialized and then either > initialize it or log a warning as appropriate. > > > > > Note that telemetry.core.util.GetBuildDirectories returns all of the build > > directories we might be interested in, not just one. > > I'm still a little confused as to this setup & how it works. > > > > > Current initialization location: > > > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... > > > > Possible gotchas: initializing devil this way means that telemetry will > > (probably) support android_mips inside a Chromium checkout, but not outside of > > one. This means we will not be running any tests on android_mips on the > catapult > > waterfall. It also doesn't give access to running on a reference build, which > is > > useful for catching performance regressions. And it still might not work in > > general cases due to Telemetry needing other platform specific dependencies > > (like memtrack_helper), or less likely because we don't correctly prefetch > > dependencies for android mips in dependency_manager.FetchBinaryDepdencies. I'm > > still in favor of adding full support - including cloud storage dependencies - > > in both devil and telemetry if android mips is a useful platform to be running > > telemetry tests on. Just supporting through a Chromium checkout seems really > > fragile. > > This would mean that telemetry would work with any locally built architecture > (i.e., x86 and x86_64, too), not just mips. Without reference builds, GCS > binaries, or automated testing, I'd be hesitant to call it official support. > > I don't think we want to add any more platforms while we're manually supporting > the GCS binaries. Once we get something automated (crbug.com/618019), perhaps we > could consider it. Here is the new patch set, could you please take a look? I've also posted patch to catapult for review, which makes use of injected devil_chromium module. Here is the link: https://codereview.chromium.org/2592803004
https://codereview.chromium.org/2573913002/diff/1/tools/perf/chrome_telemetry... File tools/perf/chrome_telemetry_build/chromium_config.py (right): https://codereview.chromium.org/2573913002/diff/1/tools/perf/chrome_telemetry... tools/perf/chrome_telemetry_build/chromium_config.py:57: devil_env.config.Initialize(config_files=[DEVIL_CONFIG_PATH]) On 2016/12/16 18:32:57, jbudorick wrote: > The right way to handle this would be to call > devil_chromium.Initialize(output_directory=...), but to do that, we'd need to > get the chromium output directory down here. Done. https://codereview.chromium.org/2573913002/diff/1/tools/perf/chrome_telemetry... tools/perf/chrome_telemetry_build/chromium_config.py:57: devil_env.config.Initialize(config_files=[DEVIL_CONFIG_PATH]) On 2016/12/16 18:39:22, aiolos wrote: > This should instead be done in telemetry's binary manager, which knows about the > Chromium src dir (and therefore the output dir) if we're in a Chromium checkout. Done.
The CQ bit was checked by djordje.golubovic@imgtec.com 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: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Posted another patch set. Please take a look.
eyaich@chromium.org changed reviewers: - eyaich@chromium.org
ping
On 2017/01/20 15:55:51, djordje.golubovic wrote: > ping Sorry about the delay. This looks fine to me, but I'll defer to one of the other two for final approval.
https://codereview.chromium.org/2573913002/diff/40001/tools/perf/chrome_telem... File tools/perf/chrome_telemetry_build/chromium_config.py (right): https://codereview.chromium.org/2573913002/diff/40001/tools/perf/chrome_telem... tools/perf/chrome_telemetry_build/chromium_config.py:56: devil_chromium.Initialize(output_directory=chromium_out_dir) Hmhh, what is this for?
On 2017/01/20 16:02:12, nednguyen wrote: > https://codereview.chromium.org/2573913002/diff/40001/tools/perf/chrome_telem... > File tools/perf/chrome_telemetry_build/chromium_config.py (right): > > https://codereview.chromium.org/2573913002/diff/40001/tools/perf/chrome_telem... > tools/perf/chrome_telemetry_build/chromium_config.py:56: > devil_chromium.Initialize(output_directory=chromium_out_dir) > Hmhh, what is this for? Also if this is a devil dependencies, you're supposed to add it to https://github.com/catapult-project/catapult/blob/master/devil/devil/devil_de... Telemetry is also supposed to fetch the devil dependencies already in https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...
https://codereview.chromium.org/2573913002/diff/40001/tools/perf/chrome_telem... File tools/perf/chrome_telemetry_build/chromium_config.py (right): https://codereview.chromium.org/2573913002/diff/40001/tools/perf/chrome_telem... tools/perf/chrome_telemetry_build/chromium_config.py:56: devil_chromium.Initialize(output_directory=chromium_out_dir) On 2017/01/20 16:02:11, nednguyen wrote: > Hmhh, what is this for? This tells devil where to find local build products (e.g. the forwarder).
https://codereview.chromium.org/2573913002/diff/40001/tools/perf/chrome_telem... File tools/perf/chrome_telemetry_build/chromium_config.py (right): https://codereview.chromium.org/2573913002/diff/40001/tools/perf/chrome_telem... tools/perf/chrome_telemetry_build/chromium_config.py:56: devil_chromium.Initialize(output_directory=chromium_out_dir) On 2017/01/20 16:04:54, jbudorick wrote: > On 2017/01/20 16:02:11, nednguyen wrote: > > Hmhh, what is this for? > > This tells devil where to find local build products (e.g. the forwarder). It either already be done in https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... or the devil config should be merged to those default configs used by Telemetry
On 2017/01/20 16:08:50, nednguyen wrote: > https://codereview.chromium.org/2573913002/diff/40001/tools/perf/chrome_telem... > File tools/perf/chrome_telemetry_build/chromium_config.py (right): > > https://codereview.chromium.org/2573913002/diff/40001/tools/perf/chrome_telem... > tools/perf/chrome_telemetry_build/chromium_config.py:56: > devil_chromium.Initialize(output_directory=chromium_out_dir) > On 2017/01/20 16:04:54, jbudorick wrote: > > On 2017/01/20 16:02:11, nednguyen wrote: > > > Hmhh, what is this for? > > > > This tells devil where to find local build products (e.g. the forwarder). > > It either already be done in > https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... > or the devil config should be merged to those default configs used by Telemetry AFAIU none of those options pick up local build products.
On 2017/01/20 16:10:15, jbudorick wrote: > On 2017/01/20 16:08:50, nednguyen wrote: > > > https://codereview.chromium.org/2573913002/diff/40001/tools/perf/chrome_telem... > > File tools/perf/chrome_telemetry_build/chromium_config.py (right): > > > > > https://codereview.chromium.org/2573913002/diff/40001/tools/perf/chrome_telem... > > tools/perf/chrome_telemetry_build/chromium_config.py:56: > > devil_chromium.Initialize(output_directory=chromium_out_dir) > > On 2017/01/20 16:04:54, jbudorick wrote: > > > On 2017/01/20 16:02:11, nednguyen wrote: > > > > Hmhh, what is this for? > > > > > > This tells devil where to find local build products (e.g. the forwarder). > > > > It either already be done in > > > https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... > > or the devil config should be merged to those default configs used by > Telemetry > > AFAIU none of those options pick up local build products. Ah, nvm. I didn't notice that these are locally built files. Though this solution seems very hacky to me. Passing the knowledge of local build directory from Telemetry to devil should be done in Telemetry core, not in client code like this. Otherwise we are having double intialization of a same module & it's very unclear to me what are the effects of that. I suggest changing https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... to pass the chromium_out dir to devil.
On 2017/01/20 16:17:34, nednguyen wrote: > On 2017/01/20 16:10:15, jbudorick wrote: > > On 2017/01/20 16:08:50, nednguyen wrote: > > > > > > https://codereview.chromium.org/2573913002/diff/40001/tools/perf/chrome_telem... > > > File tools/perf/chrome_telemetry_build/chromium_config.py (right): > > > > > > > > > https://codereview.chromium.org/2573913002/diff/40001/tools/perf/chrome_telem... > > > tools/perf/chrome_telemetry_build/chromium_config.py:56: > > > devil_chromium.Initialize(output_directory=chromium_out_dir) > > > On 2017/01/20 16:04:54, jbudorick wrote: > > > > On 2017/01/20 16:02:11, nednguyen wrote: > > > > > Hmhh, what is this for? > > > > > > > > This tells devil where to find local build products (e.g. the forwarder). > > > > > > It either already be done in > > > > > > https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... > > > or the devil config should be merged to those default configs used by > > Telemetry > > > > AFAIU none of those options pick up local build products. > > Ah, nvm. I didn't notice that these are locally built files. Though this > solution seems very hacky to me. Passing the knowledge of local build directory > from Telemetry to devil should be done in Telemetry core, not in client code > like this. Otherwise we are having double intialization of a same module & it's > very unclear to me what are the effects of that. > As noted earlier in this review, double initialization of devil_env is safe. > I suggest changing > https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... > to pass the chromium_out dir to devil.
On 2017/01/20 16:20:33, jbudorick wrote: > On 2017/01/20 16:17:34, nednguyen wrote: > > On 2017/01/20 16:10:15, jbudorick wrote: > > > On 2017/01/20 16:08:50, nednguyen wrote: > > > > > > > > > > https://codereview.chromium.org/2573913002/diff/40001/tools/perf/chrome_telem... > > > > File tools/perf/chrome_telemetry_build/chromium_config.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2573913002/diff/40001/tools/perf/chrome_telem... > > > > tools/perf/chrome_telemetry_build/chromium_config.py:56: > > > > devil_chromium.Initialize(output_directory=chromium_out_dir) > > > > On 2017/01/20 16:04:54, jbudorick wrote: > > > > > On 2017/01/20 16:02:11, nednguyen wrote: > > > > > > Hmhh, what is this for? > > > > > > > > > > This tells devil where to find local build products (e.g. the > forwarder). > > > > > > > > It either already be done in > > > > > > > > > > https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... > > > > or the devil config should be merged to those default configs used by > > > Telemetry > > > > > > AFAIU none of those options pick up local build products. > > > > Ah, nvm. I didn't notice that these are locally built files. Though this > > solution seems very hacky to me. Passing the knowledge of local build > directory > > from Telemetry to devil should be done in Telemetry core, not in client code > > like this. Otherwise we are having double intialization of a same module & > it's > > very unclear to me what are the effects of that. > > > > As noted earlier in this review, double initialization of devil_env is safe. > > > I suggest changing > > > https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... > > to pass the chromium_out dir to devil. It's safe but the semantic seems unclear to me. What happened when: devil.Initialize() devil.Initialize(foo) devil.Initialize(bar) Does "bar" override "foo" or does devil ignore bar since it's already initialized with foo or empty? I would want Telemetry not to rely on the fact that devil supports multiple initializations to keep the code easy to reason.
On 2017/01/20 16:26:03, nednguyen wrote: > On 2017/01/20 16:20:33, jbudorick wrote: > > On 2017/01/20 16:17:34, nednguyen wrote: > > > On 2017/01/20 16:10:15, jbudorick wrote: > > > > On 2017/01/20 16:08:50, nednguyen wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2573913002/diff/40001/tools/perf/chrome_telem... > > > > > File tools/perf/chrome_telemetry_build/chromium_config.py (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2573913002/diff/40001/tools/perf/chrome_telem... > > > > > tools/perf/chrome_telemetry_build/chromium_config.py:56: > > > > > devil_chromium.Initialize(output_directory=chromium_out_dir) > > > > > On 2017/01/20 16:04:54, jbudorick wrote: > > > > > > On 2017/01/20 16:02:11, nednguyen wrote: > > > > > > > Hmhh, what is this for? > > > > > > > > > > > > This tells devil where to find local build products (e.g. the > > forwarder). > > > > > > > > > > It either already be done in > > > > > > > > > > > > > > > https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... > > > > > or the devil config should be merged to those default configs used by > > > > Telemetry > > > > > > > > AFAIU none of those options pick up local build products. > > > > > > Ah, nvm. I didn't notice that these are locally built files. Though this > > > solution seems very hacky to me. Passing the knowledge of local build > > directory > > > from Telemetry to devil should be done in Telemetry core, not in client code > > > like this. Otherwise we are having double intialization of a same module & > > it's > > > very unclear to me what are the effects of that. > > > > > > > As noted earlier in this review, double initialization of devil_env is safe. > > > > > I suggest changing > > > > > > https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... > > > to pass the chromium_out dir to devil. > > It's safe but the semantic seems unclear to me. What happened when: > devil.Initialize() > devil.Initialize(foo) > devil.Initialize(bar) > > Does "bar" override "foo" or does devil ignore bar since it's already > initialized with foo or empty? Ignores. Only the first one counts. > > I would want Telemetry not to rely on the fact that devil supports multiple > initializations to keep the code easy to reason.
Does this mean it is ok?
On 2017/01/24 15:57:37, djordje.golubovic wrote: > Does this mean it is ok? not ok to me. You need to reconcile the call to initialize devil lib in tools/perf/chrome_telemetry_build/chromium_config.py to https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/....
On 2017/01/24 16:05:55, nednguyen wrote: > On 2017/01/24 15:57:37, djordje.golubovic wrote: > > Does this mean it is ok? > > not ok to me. You need to reconcile the call to initialize devil lib in > tools/perf/chrome_telemetry_build/chromium_config.py to > https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/.... Catapult can't know about devil_chromium. How do you propose such a reconciliation?
On 2017/01/24 16:06:57, jbudorick wrote: > On 2017/01/24 16:05:55, nednguyen wrote: > > On 2017/01/24 15:57:37, djordje.golubovic wrote: > > > Does this mean it is ok? > > > > not ok to me. You need to reconcile the call to initialize devil lib in > > tools/perf/chrome_telemetry_build/chromium_config.py to > > > https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/.... > > Catapult can't know about devil_chromium. How do you propose such a > reconciliation? Hmhh, I see. Then I suggest build those binaries & put them in the dependencies file in catapult: push_apps_to_background_apk go into https://github.com/catapult-project/catapult/blob/c243cc8f886b4d970654bf00937... md5sum_device & forwarder_device goes into https://github.com/catapult-project/catapult/blob/52a77489d04bbb7f8363fa80765... For each binary, we should have a consistent way to specify its location.
On 2017/01/24 16:20:42, nednguyen wrote: > On 2017/01/24 16:06:57, jbudorick wrote: > > On 2017/01/24 16:05:55, nednguyen wrote: > > > On 2017/01/24 15:57:37, djordje.golubovic wrote: > > > > Does this mean it is ok? > > > > > > not ok to me. You need to reconcile the call to initialize devil lib in > > > tools/perf/chrome_telemetry_build/chromium_config.py to > > > > > > https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/.... > > > > Catapult can't know about devil_chromium. How do you propose such a > > reconciliation? > > Hmhh, I see. Then I suggest build those binaries & put them in the dependencies > file in catapult: > push_apps_to_background_apk go into > https://github.com/catapult-project/catapult/blob/c243cc8f886b4d970654bf00937... > > md5sum_device & forwarder_device goes into > https://github.com/catapult-project/catapult/blob/52a77489d04bbb7f8363fa80765... > > For each binary, we should have a consistent way to specify its location. I maintain that we don't want to support MIPS binaries in catapult at this time.
On 2017/01/24 16:23:46, jbudorick wrote: > On 2017/01/24 16:20:42, nednguyen wrote: > > On 2017/01/24 16:06:57, jbudorick wrote: > > > On 2017/01/24 16:05:55, nednguyen wrote: > > > > On 2017/01/24 15:57:37, djordje.golubovic wrote: > > > > > Does this mean it is ok? > > > > > > > > not ok to me. You need to reconcile the call to initialize devil lib in > > > > tools/perf/chrome_telemetry_build/chromium_config.py to > > > > > > > > > > https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/.... > > > > > > Catapult can't know about devil_chromium. How do you propose such a > > > reconciliation? > > > > Hmhh, I see. Then I suggest build those binaries & put them in the > dependencies > > file in catapult: > > push_apps_to_background_apk go into > > > https://github.com/catapult-project/catapult/blob/c243cc8f886b4d970654bf00937... > > > > md5sum_device & forwarder_device goes into > > > https://github.com/catapult-project/catapult/blob/52a77489d04bbb7f8363fa80765... > > > > For each binary, we should have a consistent way to specify its location. > > I maintain that we don't want to support MIPS binaries in catapult at this time. djordje.golubovic: Ned & I are going to talk this over briefly offline; we'll get back to you.
On 2017/01/24 16:26:26, jbudorick wrote: > On 2017/01/24 16:23:46, jbudorick wrote: > > On 2017/01/24 16:20:42, nednguyen wrote: > > > On 2017/01/24 16:06:57, jbudorick wrote: > > > > On 2017/01/24 16:05:55, nednguyen wrote: > > > > > On 2017/01/24 15:57:37, djordje.golubovic wrote: > > > > > > Does this mean it is ok? > > > > > > > > > > not ok to me. You need to reconcile the call to initialize devil lib in > > > > > tools/perf/chrome_telemetry_build/chromium_config.py to > > > > > > > > > > > > > > > https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/.... > > > > > > > > Catapult can't know about devil_chromium. How do you propose such a > > > > reconciliation? > > > > > > Hmhh, I see. Then I suggest build those binaries & put them in the > > dependencies > > > file in catapult: > > > push_apps_to_background_apk go into > > > > > > https://github.com/catapult-project/catapult/blob/c243cc8f886b4d970654bf00937... > > > > > > md5sum_device & forwarder_device goes into > > > > > > https://github.com/catapult-project/catapult/blob/52a77489d04bbb7f8363fa80765... > > > > > > For each binary, we should have a consistent way to specify its location. > > > > I maintain that we don't want to support MIPS binaries in catapult at this > time. > > djordje.golubovic: Ned & I are going to talk this over briefly offline; we'll > get back to you. ok, thanks!
Ned and I weren't able to reach an agreement on what to do here. Ned feels that this shouldn't be supported in //tools/perf/ at this time, and as he's an OWNER in that directory, I defer to him. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
