Review Request 4368: Avoid using expensive bootstrap artifacts from temporary cache location

[Peiyu Wang]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4368/

Review request for pants-reviews, Benjy Weinberger, Nick Howard (Twitter), and Stu Hood. By Peiyu Wang. Repository: pants Description Problem: shading zinc is expensive, currently master shading takes 30seconds, new zinc jar is 50% larger. BaseCompileIT overrides all cache-write-to locations to temp, including bootstrap, so all will be erased. It's fine if on travis ci shard, bootstrapped artifact is already cached, this can be either leftovers from previous travis run or if another pants compile is scheduled earlier than the BaseCompileIT tests. However, if BaseCompileIT tests is scheduled to run first on the shard we are going to see every single test doing bootstrap. That's the extreme cases happening for https://rbcommons.com/s/twitter/r/4342/ Solution: Use a fixed location (buildroot/.cache) for cache-bootstrap and write everything else still to temp. Testing https://travis-ci.org/peiyuwang/pants/builds/175149101 Diffs tests/python/pants_test/backend/jvm/tasks/jvm_compile/BUILD (220431129c68b54e059cd26a31127294c3dc73f3) tests/python/pants_test/backend/jvm/tasks/jvm_compile/base_compile_integration_test.py (b7ba1859619a79b4cbb26186021887b7491fcf9a) View Diff

[Peiyu Wang]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4368/

tests/python/pants_test/backend/jvm/tasks/jvm_compile/base_compile_integration_test.py (Diff revision 1)
		72	bootstrap_cache_dir = global_subsystem_instance(CacheSetup).get_options().write_to

this is buildroot/.cache from CacheSetup.
not sure how to get scoped option value just for bootstrap, from pants.ini, that would be ~/.cache.
both serve our purpose, the latter may be slightly better so we keep files all in one place

- Peiyu Wang

On November 11th, 2016, 6:54 p.m. UTC, Peiyu Wang wrote:

Review request for pants-reviews, Benjy Weinberger, Nick Howard (Twitter), and Stu Hood. By Peiyu Wang.

Updated Nov. 11, 2016, 6:54 p.m.

[Peiyu Wang]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4368/

Review request for pants-reviews, Benjy Weinberger, Nick Howard (Twitter), and Stu Hood. By Peiyu Wang.

Updated Nov. 11, 2016, 8:57 p.m. Changes Update description. Bugs: 4047 Repository: pants Description (updated) Problem: shading zinc is expensive, currently master shading takes 30seconds, new zinc jar is 50% larger, so will take even longer. BaseCompileIT overrides all cache-write-to locations to temp, including bootstrap, so unless the default bootstrap location already contains the artifacts, every BaseCompileIT test will end up doing the expensive shading for every its pants run. One of the tests test_zinc_fatal_warning does 8 pants runs. The uncached scenario can happen when doing an zinc upgrade like in https://rbcommons.com/s/twitter/r/4342/ or on master, if travis ci shard is not locally cached (from previous travis runs), and schedules BaseCompileIT tests before other pants runs to bootstrap the default location. So this fix should help to reduce the extreme slow test cases from BaseCompileIT, sampled some previous travis runs, that seems to happen from time to time (almost anytime a shard goes > 30minutes) https://travis-ci.org/pantsbuild/pants/jobs/173113868 test_zinc_fatal_warning 516.30s https://travis-ci.org/pantsbuild/pants/jobs/173113870 test_zinc_fatal_warning 489.52s https://travis-ci.org/pantsbuild/pants/jobs/172183627 test_zinc_fatal_warning 505.19s

Solution: Use a fixed location (buildroot/.cache) for cache-bootstrap and write everything else still to temp.

Testing (updated) https://travis-ci.org/peiyuwang/pants/builds/175149101 Before on master: tw-mbp-peiyu:pants peiyu$ rm ~/.cache/pants/artifact_cache/pants_backend_jvm_tasks_bootstrap_jvm_tools_BootstrapJvmTools/.zinc/d502916b113b472095ea4ca2aa05c97c81106c91-ShadedToolFingerprintStrategy_53c4bb9c6553.tgz tw-mbp-peiyu:pants peiyu$ pt tests/python/pants_test/backend/jvm/tasks/jvm_compile/zinc:zinc_compile_integration_with_zjars --no-test-pytest-timeouts -- -k test_zinc_fatal_warning ... === 1 passed, 11 deselected in 317.94 seconds ==== After: tw-mbp-peiyu:pants peiyu$ pt tests/python/pants_test/backend/jvm/tasks/jvm_compile/zinc:zinc_compile_integration_with_zjars --no-test-pytest-timeouts -- -k test_zinc_fatal_warning ... ==== 1 passed, 11 deselected in 93.23 seconds ==== tw-mbp-peiyu:pants peiyu$ ls .cache/pants_backend_jvm_tasks_bootstrap_jvm_tools_BootstrapJvmTools/.zinc/d502916b113b472095ea4ca2aa05c97c81106c91-ShadedToolFingerprintStrategy_53c4bb9c6553.tgz .cache/pants_backend_jvm_tasks_bootstrap_jvm_tools_BootstrapJvmTools/.zinc/d502916b113b472095ea4ca2aa05c97c81106c91-ShadedToolFingerprintStrategy_53c4bb9c6553.tgz

[Benjy Weinberger]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4368/

tests/python/pants_test/backend/jvm/tasks/jvm_compile/base_compile_integration_test.py (Diff revision 1)
		71	# Use the provided cacheurl for all artifacts except for bootstrap.

Explain why we do so (because bootstrapping is expensive).

tests/python/pants_test/backend/jvm/tasks/jvm_compile/base_compile_integration_test.py (Diff revision 1)
		72	bootstrap_cache_dir = global_subsystem_instance(CacheSetup).get_options().write_to

This currently defaults to buildroot/.cache, but many of our integration tests are not hermetic - that is, they read our real pants.ini. 
So that option value could in theory be changed to something else, even a remote cache, or a list of caches, which isn't what we want in the integration tests.
However you can simply hard-code ~/.cache (after resolving ~) here if you want. There's no law that says that the value in a fake command line has to come from an option... :)

tests/python/pants_test/backend/jvm/tasks/jvm_compile/base_compile_integration_test.py (Diff revision 1)
		79	'--cache-bootstrap-read-from=[{}]'.format(format_str_list(bootstrap_cache_dir)),

Should we do this for all integration tests? Zinc is the biggest problem, but all the other ones add up too, I'm sure.

- Benjy Weinberger

On November 11th, 2016, 8:57 p.m. UTC, Peiyu Wang wrote:

Review request for pants-reviews, Benjy Weinberger, Nick Howard (Twitter), and Stu Hood. By Peiyu Wang. Updated Nov. 11, 2016, 8:57 p.m.

Bugs: 4047 Repository: pants Description Problem:

shading zinc is expensive, currently master shading takes 30seconds, new zinc jar is 50% larger, so will take even longer. BaseCompileIT overrides all cache-write-to locations to temp, including bootstrap, so unless the default bootstrap location already contains the artifacts, every BaseCompileIT test will end up doing the expensive shading for every its pants run. One of the tests test_zinc_fatal_warning does 8 pants runs. The uncached scenario can happen when doing an zinc upgrade like in https://rbcommons.com/s/twitter/r/4342/ or on master, if travis ci shard is not locally cached (from previous travis runs), and schedules BaseCompileIT tests before other pants runs to bootstrap the default location. So this fix should help to reduce the extreme slow test cases from BaseCompileIT, sampled some previous travis runs, that seems to happen from time to time (almost anytime a shard goes > 30minutes) https://travis-ci.org/pantsbuild/pants/jobs/173113868 test_zinc_fatal_warning 516.30s https://travis-ci.org/pantsbuild/pants/jobs/173113870 test_zinc_fatal_warning 489.52s https://travis-ci.org/pantsbuild/pants/jobs/172183627 test_zinc_fatal_warning 505.19s Solution: Use a fixed location (buildroot/.cache) for cache-bootstrap and write everything else still to temp.

Testing https://travis-ci.org/peiyuwang/pants/builds/175149101

Before on master: tw-mbp-peiyu:pants peiyu$ rm ~/.cache/pants/artifact_cache/pants_backend_jvm_tasks_bootstrap_jvm_tools_BootstrapJvmTools/.zinc/d502916b113b472095ea4ca2aa05c97c81106c91-ShadedToolFingerprintStrategy_53c4bb9c6553.tgz tw-mbp-peiyu:pants peiyu$ pt tests/python/pants_test/backend/jvm/tasks/jvm_compile/zinc:zinc_compile_integration_with_zjars --no-test-pytest-timeouts -- -k test_zinc_fatal_warning ... === 1 passed, 11 deselected in 317.94 seconds ==== After: tw-mbp-peiyu:pants peiyu$ pt tests/python/pants_test/backend/jvm/tasks/jvm_compile/zinc:zinc_compile_integration_with_zjars --no-test-pytest-timeouts -- -k test_zinc_fatal_warning ... ==== 1 passed, 11 deselected in 93.23 seconds ==== tw-mbp-peiyu:pants peiyu$ ls .cache/pants_backend_jvm_tasks_bootstrap_jvm_tools_BootstrapJvmTools/.zinc/d502916b113b472095ea4ca2aa05c97c81106c91-ShadedToolFingerprintStrategy_53c4bb9c6553.tgz .cache/pants_backend_jvm_tasks_bootstrap_jvm_tools_BootstrapJvmTools/.zinc/d502916b113b472095ea4ca2aa05c97c81106c91-ShadedToolFingerprintStrategy_53c4bb9c6553.tgz

[Peiyu Wang]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4368/

Review request for pants-reviews, Benjy Weinberger, Nick Howard (Twitter), and Stu Hood. By Peiyu Wang.

Updated Nov. 15, 2016, 12:31 a.m. Changes Improve comment

Bugs: 4047 Repository: pants Description

Problem:

shading zinc is expensive, currently master shading takes 30seconds, new zinc jar is 50% larger, so will take even longer. BaseCompileIT overrides all cache-write-to locations to temp, including bootstrap, so unless the default bootstrap location already contains the artifacts, every BaseCompileIT test will end up doing the expensive shading for every its pants run. One of the tests test_zinc_fatal_warning does 8 pants runs. The uncached scenario can happen when doing an zinc upgrade like in https://rbcommons.com/s/twitter/r/4342/ or on master, if travis ci shard is not locally cached (from previous travis runs), and schedules BaseCompileIT tests before other pants runs to bootstrap the default location. So this fix should help to reduce the extreme slow test cases from BaseCompileIT, sampled some previous travis runs, that seems to happen from time to time (almost anytime a shard goes > 30minutes) https://travis-ci.org/pantsbuild/pants/jobs/173113868 test_zinc_fatal_warning 516.30s https://travis-ci.org/pantsbuild/pants/jobs/173113870 test_zinc_fatal_warning 489.52s https://travis-ci.org/pantsbuild/pants/jobs/172183627 test_zinc_fatal_warning 505.19s Solution: Use a fixed location (buildroot/.cache) for cache-bootstrap and write everything else still to temp.

Testing https://travis-ci.org/peiyuwang/pants/builds/175149101

Before on master: tw-mbp-peiyu:pants peiyu$ rm ~/.cache/pants/artifact_cache/pants_backend_jvm_tasks_bootstrap_jvm_tools_BootstrapJvmTools/.zinc/d502916b113b472095ea4ca2aa05c97c81106c91-ShadedToolFingerprintStrategy_53c4bb9c6553.tgz tw-mbp-peiyu:pants peiyu$ pt tests/python/pants_test/backend/jvm/tasks/jvm_compile/zinc:zinc_compile_integration_with_zjars --no-test-pytest-timeouts -- -k test_zinc_fatal_warning ... === 1 passed, 11 deselected in 317.94 seconds ==== After: tw-mbp-peiyu:pants peiyu$ pt tests/python/pants_test/backend/jvm/tasks/jvm_compile/zinc:zinc_compile_integration_with_zjars --no-test-pytest-timeouts -- -k test_zinc_fatal_warning ... ==== 1 passed, 11 deselected in 93.23 seconds ==== tw-mbp-peiyu:pants peiyu$ ls .cache/pants_backend_jvm_tasks_bootstrap_jvm_tools_BootstrapJvmTools/.zinc/d502916b113b472095ea4ca2aa05c97c81106c91-ShadedToolFingerprintStrategy_53c4bb9c6553.tgz .cache/pants_backend_jvm_tasks_bootstrap_jvm_tools_BootstrapJvmTools/.zinc/d502916b113b472095ea4ca2aa05c97c81106c91-ShadedToolFingerprintStrategy_53c4bb9c6553.tgz

Diffs (updated)

tests/python/pants_test/backend/jvm/tasks/jvm_compile/BUILD (220431129c68b54e059cd26a31127294c3dc73f3) tests/python/pants_test/backend/jvm/tasks/jvm_compile/base_compile_integration_test.py (b7ba1859619a79b4cbb26186021887b7491fcf9a) View Diff

Show Changes

[Peiyu Wang]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4368/

On November 14th, 2016, 4:19 p.m. UTC, Benjy Weinberger wrote:

tests/python/pants_test/backend/jvm/tasks/jvm_compile/base_compile_integration_test.py (Diff revision 1)
		71	# Use the provided cacheurl for all artifacts except for bootstrap.

Explain why we do so (because bootstrapping is expensive).

Added comment.

On November 14th, 2016, 4:19 p.m. UTC, Benjy Weinberger wrote:

tests/python/pants_test/backend/jvm/tasks/jvm_compile/base_compile_integration_test.py (Diff revision 1)
		72	bootstrap_cache_dir = global_subsystem_instance(CacheSetup).get_options().write_to

This currently defaults to buildroot/.cache, but many of our integration tests are not hermetic - that is, they read our real pants.ini. 
So that option value could in theory be changed to something else, even a remote cache, or a list of caches, which isn't what we want in the integration tests.
However you can simply hard-code ~/.cache (after resolving ~) here if you want. There's no law that says that the value in a fake command line has to come from an option... :)

Loading form pants.ini sounds probalematic, was wondering if it is better. The arbitrary remote cache example makes sense.
also prefer not to hardcode, so I am going to keep as is for now: use the cache subsystem default.

On November 14th, 2016, 4:19 p.m. UTC, Benjy Weinberger wrote:

tests/python/pants_test/backend/jvm/tasks/jvm_compile/base_compile_integration_test.py (Diff revision 1)
		79	'--cache-bootstrap-read-from=[{}]'.format(format_str_list(bootstrap_cache_dir)),

Should we do this for all integration tests? Zinc is the biggest problem, but all the other ones add up too, I'm sure.

I guess the reason to use temp directory for cache in the first place is for isolation, (compile, checkstyle, ivy, etc), bugs could be tricky to debug i suppose?
whitelisting bootstrapping out is only because we can't afford to, not because we wanted to.

- Peiyu

On November 15th, 2016, 12:31 a.m. UTC, Peiyu Wang wrote:

Review request for pants-reviews, Benjy Weinberger, Nick Howard (Twitter), and Stu Hood. By Peiyu Wang.

Updated Nov. 15, 2016, 12:31 a.m. Bugs: 4047

[Benjy Weinberger]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4368/

tests/python/pants_test/backend/jvm/tasks/jvm_compile/base_compile_integration_test.py (Diff revision 1)
		72	bootstrap_cache_dir = global_subsystem_instance(CacheSetup).get_options().write_to

Ah, my point is that this is not guaranteed to take the default value. It will take the real value, whether that is the default or not.
E.g., if we happen to set that option in pants.ini and the integration test happens to be one that reads the real pants.ini, then we'll use the real value (which, again, could be a remote server or some other thing not suitable for use in integration tests).

- Benjy Weinberger

On November 15th, 2016, 12:31 a.m. UTC, Peiyu Wang wrote:

Review request for pants-reviews, Benjy Weinberger, Nick Howard (Twitter), and Stu Hood. By Peiyu Wang. Updated Nov. 15, 2016, 12:31 a.m.

Bugs: 4047

[Peiyu Wang]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4368/

On November 15th, 2016, 12:45 a.m. UTC, Benjy Weinberger wrote:

tests/python/pants_test/backend/jvm/tasks/jvm_compile/base_compile_integration_test.py (Diff revision 1)
		72	bootstrap_cache_dir = global_subsystem_instance(CacheSetup).get_options().write_to

Ah, my point is that this is not guaranteed to take the default value. It will take the real value, whether that is the default or not.
E.g., if we happen to set that option in pants.ini and the integration test happens to be one that reads the real pants.ini, then we'll use the real value (which, again, could be a remote server or some other thing not suitable for use in integration tests).

ah, i see what you mean now.
saw in the base test PantsRunIntegrationTest we have
      ini_file_name = os.path.join(workdir, 'pants.ini') => change to pants.ini.integration
      with safe_open(ini_file_name, mode='w') as fp:
        ini.write(fp)
      args.append('--config-override=' + ini_file_name)


will changing pants.ini to pants.ini.integration make this safer?
Or I will just hardcode

- Peiyu

[Peiyu Wang]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4368/

On November 15th, 2016, 12:45 a.m. UTC, Benjy Weinberger wrote:

tests/python/pants_test/backend/jvm/tasks/jvm_compile/base_compile_integration_test.py (Diff revision 1)
		72	bootstrap_cache_dir = global_subsystem_instance(CacheSetup).get_options().write_to

Ah, my point is that this is not guaranteed to take the default value. It will take the real value, whether that is the default or not.
E.g., if we happen to set that option in pants.ini and the integration test happens to be one that reads the real pants.ini, then we'll use the real value (which, again, could be a remote server or some other thing not suitable for use in integration tests).

On November 15th, 2016, 12:54 a.m. UTC, Peiyu Wang wrote:

ah, i see what you mean now.
saw in the base test PantsRunIntegrationTest we have
      ini_file_name = os.path.join(workdir, 'pants.ini') => change to pants.ini.integration
      with safe_open(ini_file_name, mode='w') as fp:
        ini.write(fp)
      args.append('--config-override=' + ini_file_name)


will changing pants.ini to pants.ini.integration make this safer?
Or I will just hardcode

I mean in pants.ini.integration we will make sure cache location is a fixed local location.

- Peiyu

[Benjy Weinberger]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4368/

On November 15th, 2016, 12:45 a.m. UTC, Benjy Weinberger wrote:

tests/python/pants_test/backend/jvm/tasks/jvm_compile/base_compile_integration_test.py (Diff revision 1)
		72	bootstrap_cache_dir = global_subsystem_instance(CacheSetup).get_options().write_to

Ah, my point is that this is not guaranteed to take the default value. It will take the real value, whether that is the default or not.
E.g., if we happen to set that option in pants.ini and the integration test happens to be one that reads the real pants.ini, then we'll use the real value (which, again, could be a remote server or some other thing not suitable for use in integration tests).

On November 15th, 2016, 12:54 a.m. UTC, Peiyu Wang wrote:

ah, i see what you mean now.
saw in the base test PantsRunIntegrationTest we have
      ini_file_name = os.path.join(workdir, 'pants.ini') => change to pants.ini.integration
      with safe_open(ini_file_name, mode='w') as fp:
        ini.write(fp)
      args.append('--config-override=' + ini_file_name)


will changing pants.ini to pants.ini.integration make this safer?
Or I will just hardcode

On November 15th, 2016, 12:56 a.m. UTC, Peiyu Wang wrote:

I mean in pants.ini.integration we will make sure cache location is a fixed local location.

Oh good point, you can just pass the value in as config to the run_pants* call.

- Benjy

[Benjy Weinberger]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4368/

tests/python/pants_test/backend/jvm/tasks/jvm_compile/base_compile_integration_test.py (Diff revision 2)
78	return self.run_pants_with_workdir(global_args + args, workdir)	88	return self.run_pants_with_workdir(global_args + args, workdir)

Basically, add config= here, and pass in the config that will set the bootstrap cache up correctly.

- Benjy Weinberger

On November 15th, 2016, 12:31 a.m. UTC, Peiyu Wang wrote:

Review request for pants-reviews, Benjy Weinberger, Nick Howard (Twitter), and Stu Hood. By Peiyu Wang. Updated Nov. 15, 2016, 12:31 a.m.

Bugs: 4047

[Peiyu Wang]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4368/

Review request for pants-reviews, Benjy Weinberger, Nick Howard (Twitter), and Stu Hood. By Peiyu Wang.

Updated Nov. 17, 2016, 8:24 p.m. Changes Simplified rb by taking advantage of config-override. Bugs: 4047 Repository: pants Description (updated)

Problem: shading zinc is expensive, currently master shading takes 30seconds, new zinc jar is 50% larger, so will take even longer. BaseCompileIT overrides all cache-write-to locations to temp, including bootstrap, so unless the default bootstrap location already contains the artifacts, every BaseCompileIT test will end up doing the expensive shading for every its pants run. One of the tests test_zinc_fatal_warning does 8 pants runs. The uncached scenario can happen when doing an zinc upgrade like in https://rbcommons.com/s/twitter/r/4342/ or on master, if travis ci shard is not locally cached (from previous travis runs), and schedules BaseCompileIT tests before other pants runs to bootstrap the default location. So this fix should help to reduce the extreme slow test cases from BaseCompileIT, sampled some previous travis runs, that seems to happen from time to time (almost anytime a shard goes > 30minutes) https://travis-ci.org/pantsbuild/pants/jobs/173113868 test_zinc_fatal_warning 516.30s https://travis-ci.org/pantsbuild/pants/jobs/173113870 test_zinc_fatal_warning 489.52s https://travis-ci.org/pantsbuild/pants/jobs/172183627 test_zinc_fatal_warning 505.19s Solution:

Use a fixed location (buildroot/.cache) for cache-bootstrap

and write everything else still to temp (by taking advantage passing cache-write-to param through override pants.ini instead of cmdline, the latter takes precedence over all pants.ini cache configurations, the former just merge therefore keep the default bootstrap cache location) Testing (updated) https://travis-ci.org/peiyuwang/pants/builds/175149101 https://travis-ci.org/peiyuwang/pants/builds/176813195

Before on master: tw-mbp-peiyu:pants peiyu$ rm ~/.cache/pants/artifact_cache/pants_backend_jvm_tasks_bootstrap_jvm_tools_BootstrapJvmTools/.zinc/d502916b113b472095ea4ca2aa05c97c81106c91-ShadedToolFingerprintStrategy_53c4bb9c6553.tgz tw-mbp-peiyu:pants peiyu$ pt tests/python/pants_test/backend/jvm/tasks/jvm_compile/zinc:zinc_compile_integration_with_zjars --no-test-pytest-timeouts -- -k test_zinc_fatal_warning ... === 1 passed, 11 deselected in 317.94 seconds ==== After: tw-mbp-peiyu:pants peiyu$ pt tests/python/pants_test/backend/jvm/tasks/jvm_compile/zinc:zinc_compile_integration_with_zjars --no-test-pytest-timeouts -- -k test_zinc_fatal_warning ... ==== 1 passed, 11 deselected in 93.23 seconds ==== tw-mbp-peiyu:pants peiyu$ ls .cache/pants_backend_jvm_tasks_bootstrap_jvm_tools_BootstrapJvmTools/.zinc/d502916b113b472095ea4ca2aa05c97c81106c91-ShadedToolFingerprintStrategy_53c4bb9c6553.tgz .cache/pants_backend_jvm_tasks_bootstrap_jvm_tools_BootstrapJvmTools/.zinc/d502916b113b472095ea4ca2aa05c97c81106c91-ShadedToolFingerprintStrategy_53c4bb9c6553.tgz

Diffs (updated) tests/python/pants_test/backend/jvm/tasks/jvm_compile/base_compile_integration_test.py (291675fc40e4ad69dc193e3483368302d6a3b0a7) View Diff Show Changes

[Peiyu Wang]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4368/

On November 16th, 2016, 11:26 p.m. UTC, Benjy Weinberger wrote:

tests/python/pants_test/backend/jvm/tasks/jvm_compile/base_compile_integration_test.py (Diff revision 2)
78	return self.run_pants_with_workdir(global_args + args, workdir)	88	return self.run_pants_with_workdir(global_args + args, workdir)

Basically, add config= here, and pass in the config that will set the bootstrap cache up correctly.

Observed the following difference when passing options from cmdline vs config-override:
tw-mbp-peiyu:pants peiyu$ ./pants --no-colors --cache-write --cache-write-to="['/tmp/peiyu']" options|egrep "^cache."
cache.read = True (from HARDCODED)
cache.read_from = [] (from CONFIG)
cache.write = True (from HARDCODED)
cache.write_to = ['/tmp/peiyu'] (from FLAG)
cache.bootstrap.read_from = ['/Users/peiyu/.cache/pants/artifact_cache'] (from CONFIG)


tw-mbp-peiyu:pants peiyu$ cat /tmp/pants.ini
[cache]
write = True
write_to = ['/tmp/peiyu']

tw-mbp-peiyu:pants peiyu$ ./pants --no-colors --config-override=/tmp/pants.ini options|egrep "^cache."
cache.read = True (from HARDCODED)
cache.read_from = [] (from CONFIG)
cache.write = True (from HARDCODED)
cache.write_to = ['/tmp/peiyu'] (from CONFIG)
cache.bootstrap.read_from = ['/Users/peiyu/.cache/pants/artifact_cache'] (from CONFIG)
cache.bootstrap.write_to = ['/Users/peiyu/.cache/pants/artifact_cache'] (from CONFIG)


Note the extra cache.bootstrap.write_to is preserved in the latter, when merging config-override with pants.ini, but not through FLAG in the former.
Because of that, all I need is switch to convert cmdline cache options to config.
Updated rb, let me know if that behavior is not intended.

- Peiyu

On November 17th, 2016, 8:24 p.m. UTC, Peiyu Wang wrote:

Review request for pants-reviews, Benjy Weinberger, Nick Howard (Twitter), and Stu Hood. By Peiyu Wang.

Updated Nov. 17, 2016, 8:24 p.m. Bugs: 4047 Repository: pants Description

Problem: shading zinc is expensive, currently master shading takes 30seconds, new zinc jar is 50% larger, so will take even longer. BaseCompileIT overrides all cache-write-to locations to temp, including bootstrap, so unless the default bootstrap location already contains the artifacts, every BaseCompileIT test will end up doing the expensive shading for every its pants run. One of the tests test_zinc_fatal_warning does 8 pants runs. The uncached scenario can happen when doing an zinc upgrade like in https://rbcommons.com/s/twitter/r/4342/ or on master, if travis ci shard is not locally cached (from previous travis runs), and schedules BaseCompileIT tests before other pants runs to bootstrap the default location. So this fix should help to reduce the extreme slow test cases from BaseCompileIT, sampled some previous travis runs, that seems to happen from time to time (almost anytime a shard goes > 30minutes) https://travis-ci.org/pantsbuild/pants/jobs/173113868 test_zinc_fatal_warning 516.30s https://travis-ci.org/pantsbuild/pants/jobs/173113870 test_zinc_fatal_warning 489.52s https://travis-ci.org/pantsbuild/pants/jobs/172183627 test_zinc_fatal_warning 505.19s Solution:

Use a fixed location (buildroot/.cache) for cache-bootstrap and write everything else still to temp (by taking advantage passing cache-write-to param through override pants.ini instead of cmdline, the latter takes precedence over all pants.ini cache configurations, the former just merge therefore keep the default bootstrap cache location)

Testing https://travis-ci.org/peiyuwang/pants/builds/175149101 https://travis-ci.org/peiyuwang/pants/builds/176813195

Before on master: tw-mbp-peiyu:pants peiyu$ rm ~/.cache/pants/artifact_cache/pants_backend_jvm_tasks_bootstrap_jvm_tools_BootstrapJvmTools/.zinc/d502916b113b472095ea4ca2aa05c97c81106c91-ShadedToolFingerprintStrategy_53c4bb9c6553.tgz tw-mbp-peiyu:pants peiyu$ pt tests/python/pants_test/backend/jvm/tasks/jvm_compile/zinc:zinc_compile_integration_with_zjars --no-test-pytest-timeouts -- -k test_zinc_fatal_warning ... === 1 passed, 11 deselected in 317.94 seconds ==== After: tw-mbp-peiyu:pants peiyu$ pt tests/python/pants_test/backend/jvm/tasks/jvm_compile/zinc:zinc_compile_integration_with_zjars --no-test-pytest-timeouts -- -k test_zinc_fatal_warning ... ==== 1 passed, 11 deselected in 93.23 seconds ==== tw-mbp-peiyu:pants peiyu$ ls .cache/pants_backend_jvm_tasks_bootstrap_jvm_tools_BootstrapJvmTools/.zinc/d502916b113b472095ea4ca2aa05c97c81106c91-ShadedToolFingerprintStrategy_53c4bb9c6553.tgz .cache/pants_backend_jvm_tasks_bootstrap_jvm_tools_BootstrapJvmTools/.zinc/d502916b113b472095ea4ca2aa05c97c81106c91-ShadedToolFingerprintStrategy_53c4bb9c6553.tgz

Diffs tests/python/pants_test/backend/jvm/tasks/jvm_compile/base_compile_integration_test.py (291675fc40e4ad69dc193e3483368302d6a3b0a7) View Diff

[Benjy Weinberger]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4368/

Ship it!

Nice speed boost!

- Benjy Weinberger

On November 17th, 2016, 8:24 p.m. UTC, Peiyu Wang wrote:

Review request for pants-reviews, Benjy Weinberger, Nick Howard (Twitter), and Stu Hood. By Peiyu Wang. Updated Nov. 17, 2016, 8:24 p.m.

Bugs: 4047 Repository: pants Description Problem:

shading zinc is expensive, currently master shading takes 30seconds, new zinc jar is 50% larger, so will take even longer. BaseCompileIT overrides all cache-write-to locations to temp, including bootstrap, so unless the default bootstrap location already contains the artifacts, every BaseCompileIT test will end up doing the expensive shading for every its pants run. One of the tests test_zinc_fatal_warning does 8 pants runs. The uncached scenario can happen when doing an zinc upgrade like in https://rbcommons.com/s/twitter/r/4342/ or on master, if travis ci shard is not locally cached (from previous travis runs), and schedules BaseCompileIT tests before other pants runs to bootstrap the default location. So this fix should help to reduce the extreme slow test cases from BaseCompileIT, sampled some previous travis runs, that seems to happen from time to time (almost anytime a shard goes > 30minutes) https://travis-ci.org/pantsbuild/pants/jobs/173113868 test_zinc_fatal_warning 516.30s https://travis-ci.org/pantsbuild/pants/jobs/173113870 test_zinc_fatal_warning 489.52s https://travis-ci.org/pantsbuild/pants/jobs/172183627 test_zinc_fatal_warning 505.19s Solution: Use a fixed location (buildroot/.cache) for cache-bootstrap and write everything else still to temp (by taking advantage passing cache-write-to param through override pants.ini instead of cmdline, the latter takes precedence over all pants.ini cache configurations, the former just merge therefore keep the default bootstrap cache location)

Testing https://travis-ci.org/peiyuwang/pants/builds/175149101 https://travis-ci.org/peiyuwang/pants/builds/176813195

Before on master: tw-mbp-peiyu:pants peiyu$ rm ~/.cache/pants/artifact_cache/pants_backend_jvm_tasks_bootstrap_jvm_tools_BootstrapJvmTools/.zinc/d502916b113b472095ea4ca2aa05c97c81106c91-ShadedToolFingerprintStrategy_53c4bb9c6553.tgz tw-mbp-peiyu:pants peiyu$ pt tests/python/pants_test/backend/jvm/tasks/jvm_compile/zinc:zinc_compile_integration_with_zjars --no-test-pytest-timeouts -- -k test_zinc_fatal_warning ... === 1 passed, 11 deselected in 317.94 seconds ==== After: tw-mbp-peiyu:pants peiyu$ pt tests/python/pants_test/backend/jvm/tasks/jvm_compile/zinc:zinc_compile_integration_with_zjars --no-test-pytest-timeouts -- -k test_zinc_fatal_warning ... ==== 1 passed, 11 deselected in 93.23 seconds ==== tw-mbp-peiyu:pants peiyu$ ls .cache/pants_backend_jvm_tasks_bootstrap_jvm_tools_BootstrapJvmTools/.zinc/d502916b113b472095ea4ca2aa05c97c81106c91-ShadedToolFingerprintStrategy_53c4bb9c6553.tgz .cache/pants_backend_jvm_tasks_bootstrap_jvm_tools_BootstrapJvmTools/.zinc/d502916b113b472095ea4ca2aa05c97c81106c91-ShadedToolFingerprintStrategy_53c4bb9c6553.tgz

Diffs tests/python/pants_test/backend/jvm/tasks/jvm_compile/base_compile_integration_test.py (291675fc40e4ad69dc193e3483368302d6a3b0a7) View Diff