Add S3 Cache to pants #4589

toddgardner · 2017-05-14T23:34:57Z

Problem

This adds an s3 implementation of the pants cache. S3 is used as it is simpler for smaller companies to maintain than running and securing another server and potential caching layer, and configuring development laptops, etc.

Solution

This is an implementation written for my previous startup by @JoeEnnever that we're pushing upstream. It's got several practical tweaks like abandoning on errors, enforcing timeouts, etc.

The advantage of not having this as a plugin is it avoids having to monkey patch cache_setup, or build a new plugin system just for cache backends. Additionally there are some problems installing boto in a plugin, because it isn't zipsafe (#4428).

The downsides is that it adds two dependencies to pants core, in terms of boto3 and pyjavaproperties.

It has several ways to pass credentials, including standard boto chain, with an optionally specified profile.

It also includes a way to have the properties come from a java-properties file. This is useful if your company uses s3 as an ivy artifact storage as well; you can share the credentials between pants and ivy xml. It's unclear if this is a wide enough use-case to actually push upstream.

I also haven't done any performance testing on this versus actually just rebuilding targets. It doesn't seem to feel slow, at least.

stuhood · 2017-05-15T01:17:58Z

Thanks Todd! Not sure why Travis isn't kicking off... perhaps because this is your first PR. Will review tomorrow!

JoeEnnever · 2017-05-15T14:13:00Z

src/python/pants/cache/s3_artifact_cache.py

+    # Delegate storage and extraction to local cache
+    try:
+      return self._localcache.store_and_use_artifact(
+        cache_key, iter_content(get_result['Body']), results_dir)


This needs to close get_result['Body'], otherwise python holds onto the file contents in memory.

Ah ha, that's your fabled memory leak. Done

stuhood

Can you clarify the help string for the --resolver option in the context of this new capability? For example, indicating that the resolver is only relevant to http(s) URLs.

stuhood

Looks good to me.

I'd like a second opinion on the risk of adding boto as a dependency, but... seems low?

toddgardner · 2017-05-15T21:20:08Z

Changed resolver flag, waiting on a response from travis support.

mateor

I am super excited for this. I spoke with AWS about Pants support last week and we discussed how we would probably want to write this interface someday.

Code looks good too. I have some suggestions and some nits. The biggest effort ask is the switch from pytest to unittest. If Stu doesn't mind pytest creeping back in, I will not block on it.

Thanks Todd!

mateor · 2017-05-16T15:42:33Z

3rdparty/python/requirements.txt

@@ -1,5 +1,6 @@
 ansicolors==1.0.2
 beautifulsoup4>=4.3.2,<4.4
+boto3==1.4.4


boto is for sure the way to go - not a concern for me.

I have no problem with this dep.

mateor · 2017-05-16T15:49:17Z

src/python/pants/cache/cache_setup.py

@@ -84,6 +85,12 @@ def register_options(cls, register):
             help='number of times pinger tries a cache')
    register('--write-permissions', advanced=True, type=str, default=None,
             help='Permissions to use when writing artifacts to a local cache, in octal.')
+    register('--s3-credential-file', advanced=True, type=str,
+             default=os.path.expanduser('~/.pants/.s3credentials'),


There is a pants_configdir option that this should use instead.

(Pdb) self.get_options().pants_configdir '/Users/mateo/.config/pants'

mateor · 2017-05-16T15:50:52Z

src/python/pants/cache/s3_artifact_cache.py

@@ -0,0 +1,178 @@
+# coding=utf-8
+# Copyright 2014 Pants project contributors (see CONTRIBUTORS.md).


nit: copyright year

mateor · 2017-05-16T15:53:38Z

src/python/pants/cache/s3_artifact_cache.py

+  exceptions.EndpointConnectionError, exceptions.ChecksumError
+]
+
+def _connect_to_s3(config_file, profile_name):


I wrote something similar, android_config_util.py, as an intern - at some point there should be a general interface for non-Pants config reads for secrets like these.

Not a problem for this review, though

mateor · 2017-05-16T16:03:39Z

src/python/pants/cache/s3_artifact_cache.py

+    s3_object = self._get_object(cache_key)
+    try:
+      get_result = s3_object.get()
+    except Exception as e:


Be nice to catch a tighter exception here, but I think staying in sync with what the local_artifact_cache does was probably the right choice.

mateor · 2017-05-16T16:11:30Z

src/python/pants/cache/s3_artifact_cache.py

+  def _get_object(self, cache_key):
+    return self._s3.Object(self._bucket, self._path_for_key(cache_key))
+
+  def _path_for_key(self, cache_key):


Could you move _cache_file_for_key up to the base class instead please?

mateor · 2017-05-16T16:11:59Z

tests/python/pants_test/cache/test_s3_artifact_cache.py

@@ -0,0 +1,198 @@
+# coding=utf-8
+# Copyright 2014 Pants project contributors (see CONTRIBUTORS.md).


nit: year again

mateor · 2017-05-16T16:13:45Z

tests/python/pants_test/cache/test_s3_artifact_cache.py

+from contextlib import contextmanager
+
+import boto3
+import pytest


It would be best to not import pytest here and use unittest like the rest of the codebase.

There was some effort put into ripping pytest out and I don't see any remaining usage in the Pants tests tree

Ah, didn't realize this was an intentional choice in pants. I'm fine with rewriting it (and doing the rest of these changes); will take a stab after work.

I am excited about this contribution, glad you (re)joined the Pants community :)

Feel free to ping me if you need a committer/reviewer.

It was an intentional choice, although not necessarily the right one. Pytest certainly encourages you not to, and there are some features unavailable to class-based tests. Nonetheless, fixtures are confusing, and xtest-style tests are well understood by most, so...

benjyw

This is an awesome change, that will increase Pants's appeal (assuming latency to S3 is acceptable in a build environment). Thanks for this! Just a few comments inline.

benjyw · 2017-05-18T14:52:30Z

3rdparty/python/requirements.txt

@@ -1,5 +1,6 @@
 ansicolors==1.0.2
 beautifulsoup4>=4.3.2,<4.4
+boto3==1.4.4


I have no problem with this dep.

benjyw · 2017-05-18T14:56:19Z

src/python/pants/cache/s3_artifact_cache.py

+        logger.debug('Reading access key from {0}'.format(config_file))
+        boto_kwargs['aws_secret_access_key'] = secret_key
+  except IOError:
+    logger.debug('Could not load {0}, using ENV vars'.format(config_file))


Let's add the IOError's message to the log message, so the user can figure out what happened.

Also, should this be a warn? Or is this expected behavior in some cases? If the latter, it seems like asking for trouble to be unable to distinguish between expected behavior and an error... Perhaps we should expect that the config_file, if not None, must be readable?

benjyw · 2017-05-18T14:59:26Z

src/python/pants/cache/s3_artifact_cache.py

+    logger.debug('Could not load {0}, using ENV vars'.format(config_file))
+
+  session = boto3.Session(**boto_kwargs)
+  config = Config(connect_timeout=4, read_timeout=4)


Should these be options?

benjyw · 2017-05-18T15:00:11Z

src/python/pants/cache/s3_artifact_cache.py

+  return session.resource('s3', config=config)
+
+
+_READ_SIZE_BYTES = 4 * 1024 * 1024


Should this be an option? Or is it just super-standard to read 4MB chunks from S3?

benjyw · 2017-05-18T15:00:57Z

src/python/pants/cache/s3_artifact_cache.py

+
+  def try_insert(self, cache_key, paths):
+    logger.debug('Insert {0}'.format(cache_key))
+    # Delegate creation of artifacts to the local cache


Nit: End sentence with a period.

benjyw · 2017-05-18T15:04:28Z

tests/python/pants_test/cache/test_s3_artifact_cache.py

+from contextlib import contextmanager
+
+import boto3
+import pytest


It was an intentional choice, although not necessarily the right one. Pytest certainly encourages you not to, and there are some features unavailable to class-based tests. Nonetheless, fixtures are confusing, and xtest-style tests are well understood by most, so...

benjyw · 2017-05-18T15:04:58Z

tests/python/pants_test/cache/test_s3_artifact_cache.py

+_TEST_BUCKET = 'verst-test-bucket'
+
+
+@pytest.yield_fixture(scope="function")


Nit: single quotes, here and all other fixtures below. Although I guess it doesn't matter since you're going to rewrite this anyway.

stuhood · 2017-07-19T18:08:13Z

@toddgardner : Do you have time to wrap this one up? It would be really appreciated!

mateor · 2017-09-04T10:24:42Z

I am going to experiment with this. If the latencies are workable, I can possibly carry the work forward if Todd doesn't have the bandwidth

mateor · 2017-09-05T18:28:31Z

experiment was positive, will move forward unless Todd objects

toddgardner

Sorry, I just came back from vacation. This looks great, thanks for picking it up.

mateor · 2017-09-07T13:04:06Z

Okay, will do. We just converted a cache to s3 and will be experimenting. Once that looks reasonable, I will push a PR.

Eric-Arellano · 2020-05-17T21:12:06Z

Closing because caching has changed dramatically with the V2 implementation of Pants to now require gRPC.

Thank you for originally putting up this change, though!

Add S3 Cache to pants

e34fde5

stuhood requested review from stuhood, benjyw and wisechengyi May 15, 2017 01:17

JoeEnnever reviewed May 15, 2017

View reviewed changes

stuhood reviewed May 15, 2017

View reviewed changes

stuhood approved these changes May 15, 2017

View reviewed changes

Respond to PR comments

5455869

Small change to rerun travis

3623239

mateor suggested changes May 16, 2017

View reviewed changes

benjyw requested changes May 18, 2017

View reviewed changes

toddgardner commented Sep 6, 2017

View reviewed changes

jsirois mentioned this pull request Sep 7, 2017

SHARD="Python integration tests for pants - shard 1" runtime can exceed travis timeout #4850

Closed

stuhood force-pushed the master branch from b6bb42d to 9e2fdb5 Compare May 11, 2018 23:54

Eric-Arellano closed this May 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add S3 Cache to pants #4589

Add S3 Cache to pants #4589

toddgardner commented May 14, 2017

stuhood commented May 15, 2017

JoeEnnever May 15, 2017

toddgardner May 15, 2017

stuhood left a comment

stuhood left a comment

toddgardner commented May 15, 2017

mateor left a comment

mateor May 16, 2017

benjyw May 18, 2017

mateor May 16, 2017

mateor May 16, 2017

mateor May 16, 2017

mateor May 16, 2017

mateor May 16, 2017

mateor May 16, 2017 •

edited by benjyw

mateor May 16, 2017

toddgardner May 16, 2017

mateor May 18, 2017

benjyw May 18, 2017

benjyw left a comment

benjyw May 18, 2017

benjyw May 18, 2017

benjyw May 18, 2017

benjyw May 18, 2017

benjyw May 18, 2017

benjyw May 18, 2017

benjyw May 18, 2017

stuhood commented Jul 19, 2017

mateor commented Sep 4, 2017

mateor commented Sep 5, 2017

toddgardner left a comment

mateor commented Sep 7, 2017

Eric-Arellano commented May 17, 2020

		@@ -0,0 +1,178 @@
		# coding=utf-8
		# Copyright 2014 Pants project contributors (see CONTRIBUTORS.md).

		@@ -0,0 +1,198 @@
		# coding=utf-8
		# Copyright 2014 Pants project contributors (see CONTRIBUTORS.md).

		return session.resource('s3', config=config)


		_READ_SIZE_BYTES = 4 * 1024 * 1024

		_TEST_BUCKET = 'verst-test-bucket'


		@pytest.yield_fixture(scope="function")

Add S3 Cache to pants #4589

Add S3 Cache to pants #4589

Conversation

toddgardner commented May 14, 2017

Problem

Solution

stuhood commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

toddgardner commented May 15, 2017

mateor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateor May 16, 2017 • edited by benjyw

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjyw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood commented Jul 19, 2017

mateor commented Sep 4, 2017

mateor commented Sep 5, 2017

toddgardner left a comment

Choose a reason for hiding this comment

mateor commented Sep 7, 2017

Eric-Arellano commented May 17, 2020

mateor May 16, 2017 •

edited by benjyw