Avoid using expensive bootstrap artifacts from temporary cache location

Review Request #4368 — Created Nov. 11, 2016 and submitted

peiyu
pants
4047
pants-reviews
benjyw, nhoward_tw, stuhood

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)

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
  • 0
  • 0
  • 3
  • 1
  • 4
Description From Last Updated
PE
  1. 
      
  2. 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

  3. 
      
PE
BE
  1. 
      
  2. Explain why we do so (because bootstrapping is expensive).

  3. 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... :)

    1. 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.

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

    1. 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.

  5. 
      
PE
BE
  1. 
      
  2. 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).

    1. 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

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

    3. Oh good point, you can just pass the value in as config to the run_pants* call.
  3. 
      
BE
  1. 
      
  2. Basically, add config= here, and pass in the config that will set the bootstrap cache up correctly.

    1. 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.

  3. 
      
PE
BE
  1. Nice speed boost!

  2. 
      
PE
Review request changed

Status: Closed (submitted)

Change Summary:

Merged as eb106d9ebcc5a22fd4c439d55c0dd32a93564bc3

Loading...