Parameter Server: Run TF server by default #36

wbuchwalter · 2017-09-12T15:27:56Z

First draft for #16.

I did not provide a default value for the new tfVersion. While we could default to latest or something else, I feel like it might be difficult to figure out where the issue comes from in case of mismatch between PS and workers version.
Currently the script is only building one image for every stable minor version starting from 1.1.0. Do we want to add more releases?

I will need to change the repository where the PS default image are pushed before merging.

This change is

jlewi

Thanks for taking this on!

jlewi · 2017-09-13T12:50:56Z

images/default-ps/Dockerfile

@@ -0,0 +1,5 @@
+ARG BASE_IMAGE=tensorflow/tensorflow


I didn't realize we'd have to build our own Docker images. I thought grpc_tensorflow_server.py was already included in the standard TensorFlow image. Might be worth filing an FR for that.

jlewi · 2017-09-13T12:53:05Z

images/default-ps/main.py

@@ -0,0 +1,22 @@
+# A very simple parameter server that joins the server defined by the cluster spec passed as environment variable


Can we use grpc_tensorflow_server.py (just make a copy)? Ideally this binary is eventually part of the TensorFlow Docker image. So the less divergence from the repo the better.

jlewi · 2017-09-13T12:55:57Z

images/default-ps/build_and_push.sh

@@ -0,0 +1,22 @@
+#!/bin/bash


Can we avoid creating additional Docker images? What if we create a ConfigMap with grpc_tensorflow_server.py (or app.py) and then mount that into the PS container. I think that would allow us to us a stock TensorFlow image.

jlewi · 2017-09-13T13:06:08Z

pkg/spec/tf_job.go

@@ -91,6 +92,8 @@ type TfReplicaSpec struct {
 	// TfPort is the port to use for TF services.
 	TfPort        *int32 `json:"tfPort,omitempty" protobuf:"varint,1,opt,name=tfPort"`
 	TfReplicaType `json:"tfReplicaType"`
+	//TfVersion is only used when TfReplicaType == PS to automatically start a PS server
+	TfVersion string `json:"tfVersion,omitempty"`


Can a TfJobSpec just have an optional TfImage? This would be a Docker Image URI for a container with default binaries. Right now we have two "tensorboard" and "grpc_std_server"?

Now that I'm looking at the code I think its better if we can avoid adding a layer of indirection in the form of a mapping from Tf versions to Docker images.

wbuchwalter · 2017-09-13T14:31:50Z

@jlewi We can use https://hub.docker.com/r/tensorflow/tf_grpc_server/ instead of building our own images, but there are only basically two versions: 0.8.0 and latest last built a year ago.
Can I assume that a new image is only ever build when a breaking changes occur? Or is not really maintained?

jlewi · 2017-09-13T17:05:43Z

Can I assume that a new image is only ever build when a breaking changes occur? Or is not really maintained?

@wbuchwalter I don't think so. I think those images are outdated and unlikely to be compatible with the latest versions of TF. I don't think TF provides a lot of cross version guarantees. So ideally all Docker images in the job are derived from the same TensorFlow docker image so they are using the same version of TF.

wbuchwalter · 2017-09-14T19:49:22Z

@jlewi I implemented your suggestion and it's now using a ConfigMap to avoid having to maintain images (great idea btw).
Only downside is that the TF_CONFIG passed by tfjob as env variable is in json format, while grpc_tensorflow_server.py expects something like --cluster_spec worker|worker-74om-0:2222;worker-74om-1:2222,ps|ps-74om-0:2222;ps-74om-1:2222,master|master-74om-0:2222.
So I had to add a second python file to the ConfigMap that converts from json to this expected format and calls grpc_tensorflow_server.py with the correct argument.

Let me know if you are okay with this or if you thinks this is too brittle.

jlewi

@wbuchwalter Instead of doing the json to string conversion in Python can we do it in the controller? e.g. in the call to setDefaults you could just convert the spec to the format expected and pass it as an argument to the Python program.

jlewi · 2017-09-14T20:46:37Z

grpc_tensorflow_server/grpc_tensorflow_server.py

+# See the License for the specific language governing permissions and
+# limitations under the License.
+# ==============================================================================
+"""Python-based TensorFlow GRPC server.


Add a TODO indicating that we'd eventually like to get rid of this once grpc_tensorflow_server is included in the TF container.

wbuchwalter · 2017-09-15T14:17:45Z

@jlewi Since the setDefaults happens in the spec package even before the validation we don't know the clusterSpec at this stage (unless I missed something).
Another option would be to add a custom label to the default PS pod to identify it in setDefaults.
I could then modify the Create function in replicas.go to check if the current replicas we are creating has this label, and if yes do the appropriate conversions, and update the Command of the pod.
The advantage is that if/when TF official image includes grpc_tensorflow_server.py this approach would still work.
What do you think?

jlewi · 2017-09-15T17:47:48Z

@wbuchwalter You're right thanks for reminding me. It seems like there are two approaches

Refactor the logic for creating the ClusterSpec so that its available in setDefaults
Defer specifying the Image and Command to Create in replicas.go

There are a couple things I don't like about doing it in Create.

It creates inconsistencies between handling of PS and other replicas; i.e. other replicas are fully specified after setDefaults whereas for PS its delayed until create.
I think the actual PodTemplateSpec for each replica should end up in the TfJobSpec (I think this helps debuggability)
We could update the TfJobSpec in Create, but this would violate normal K8s conventions that spec is fixed when resource is created.

So I think my preference would be to figure out how to make cluster_spec available in setDefaults.
I think the issue is that to compute cluster_spec we need to know the port for each process which is one of the defaults that gets filled in.

One solution would be to do two passes over the replicas to fill in setDefaults. First pass would figure out the port to use for each process. Then for the second pass, we can compute cluster_spec and use that to fill out Command.

wbuchwalter · 2017-09-21T20:35:42Z

Sorry for the delay responding to this, currently travelling.
After trying different things I can't really find a clean solution to this that wouldn't need a significant refactoring.
The main issue is that grpc_tensorflow_server also needs to be passed the task_id as argument in the Command.
So that means that every replica in the TFReplicaSet needs a different Command which can't really be achieved currently before the TFReplicaSet.Create() is called.

Another option would be to be able to specify a TF_CONFIG transform function in spec that would be called just before setting the environment variable on the replica. But I'm not sure this is really clean nor easily understandable.

Unless you have other feature in mind that could benefit from this mechanism, I think the current approach (doing the transform in python at runtime) still is cleaner albeit not ideal.

What do you think? Do you see another way of doing what you said that I missed?

jlewi · 2017-09-22T04:43:35Z

Thanks for the explanation (I should have thought of that).

In that case, I think its fine to do the transformation in Create and set the command appropriately. I'd prefer not to introduce extra python code. I'd also prefer not to use PodLabels as a way of signaling to Create that it needs to set the command. Instead can we set a private variable on either TfReplicaSet or TfJob to indicate that Create needs to set the command appropriately?

wbuchwalter · 2017-09-26T18:01:50Z

@jlewi Could you give it another look?
Thanks!

jlewi

Thanks. Only one major comment regarding whether we should create a ConfigMap for each job.

jlewi · 2017-09-26T22:51:18Z

pkg/controller/controller.go

 	return watchVersion, nil
 }

+//Create a ConfigMap containing the source for a simple grpc server (pkg/controller/grpc_tensorflow_server.py)


Space after "//"

jlewi · 2017-09-26T22:58:42Z

pkg/controller/controller.go

+func (c *Controller) createPSConfigMap() error {
+	//If a ConfigMap with the same name already exists, it was created by an earlier operator
+	//we delete and recreate it in case the grpc_tensorflow_server.py was updated in the meantime
+	cm, err := c.KubeCli.CoreV1().ConfigMaps(c.Namespace).Get(spec.PSConfigMapName(), metav1.GetOptions{})


Will this cause problems? Suppose a job is happily running using the supplied config map. Now suppose the operator gets restarted. The operator would then try to delete and recreate the ConfigMap which seems problematic since some jobs could already be mounting it.

Here are some options I can think of for avoiding this.

Create a unique ConfigMap for every job

Do not delete the ConfigMap if it already exists; just reuse it.

I favor the first option because the ConfigMap is just a temporary solution until the TensorFlow container has the gRPC server binary built in. I think ConfigMap per job makes it more robust and I think this is better than the slight efficiency we get by reusing ConfigMaps.

Good catch.
1 ConfigMap per job would also mean specifying the volumes configuration for the default PS in the create phase (probably where I specify the Command) because the name of the ConfigMap won't be known in advance anymore and will rely on the RuntimeId.
Are you ok with that?

jlewi · 2017-09-26T22:59:30Z

pkg/spec/tf_job.go

@@ -91,6 +96,10 @@ type TfReplicaSpec struct {
 	// TfPort is the port to use for TF services.
 	TfPort        *int32 `json:"tfPort,omitempty" protobuf:"varint,1,opt,name=tfPort"`
 	TfReplicaType `json:"tfReplicaType"`
+	//TfImage is only used when TfReplicaType == PS to automatically start a PS server


Nit: Space after "//" here and for other comments please.

jlewi · 2017-09-26T23:00:49Z

pkg/spec/tf_job.go

@@ -75,7 +79,8 @@ const (
 type ContainerName string

 const (
-	TENSORFLOW ContainerName = "tensorflow"
+	TENSORFLOW     ContainerName = "tensorflow"
+	PsDefaultImage               = "wbuchwalter/mlkube-tensorflow-ps"


Can we default to a TensorFlow image now? Can we make this a flag to make it easily customizable?

Sorry this is something I forgot to remove from my first implementation (with custom images).
Now, if you want to use the default PS, you would declare a template like this:

- replicas: 2 tfReplicaType: PS tfImage: tensorflow/tensorflow:1.3.0

So it's already easily customizable.
I chose not to set any default for tfImage because it case of version mismatch between the PS and the other nodes it might be quite hard for the users to understand where the issue comes from.
Let me know if you would rather have a default.

So tfImage is specified at the Replica level? Why not make it a property of the job instead? This way we only have to specify it once and we can reuse it for the TensorBoard replica and the PS. If you want to specify a custom image at the Replica level you can already do that by filling out a PodTemplateSpec although it would be more verbose.

jlewi · 2017-09-26T23:03:21Z

pkg/trainer/replicas.go

@@ -88,6 +90,32 @@ func (s *TFReplicaSet) Labels() KubernetesLabels {
 		"runtime_id": s.Job.job.Spec.RuntimeId})
 }

+// Transforms the tfconfig to work with grpc_tensorflow_server
+func transformClusterSpecForDefaultPS(clusterSpec ClusterSpec) string {


Can you write a unittest for this please?

jlewi · 2017-09-26T23:04:31Z

pkg/trainer/replicas.go

+			// We do the appropriate transformations here
+			cs := transformClusterSpecForDefaultPS(s.Job.ClusterSpec())
+			s.Spec.Template.Spec.Containers[0].Command = []string{"python", "/ps-server/grpc_tensorflow_server.py"}
+			s.Spec.Template.Spec.Containers[0].Args = []string{"--cluster_spec", cs, "--job_name", "ps", "--task_id", fmt.Sprintf("%v", index)}


Why did you split it into command and args rather than just using command?

No good reason, I will change this.

jlewi · 2017-09-26T23:07:33Z

pkg/controller/controller.go

+
+	//grab server sources from files
+	filePaths := map[string]string{
+		"grpc_tensorflow_server.py": "./grpc_tensorflow_server/grpc_tensorflow_server.py",


Can we make the path of grpc_tensorflow_server.py a flag that gets plumbed through?

Do you plan on updating the Dockerfile for the controller to include grpc_tensorflow_server.py?

To make sure I get this right, do you want this to be a flag at the controller level, so users could specify a custom `grpc_tensorflow_server.py, or do you want a flag at the job level to customize where this file gets mounted in the pod?

I was thinking at the controller level not the job level. Right now the Docker image and controller both have to hardcode the same location of grpc_tensorflow_server.py. That seems brittle.

If you make it a flag in main.go that gets plumbed through, then the Docker image can put grpc_tensorflow_server.py anywhere in the image and we can just specify the location as an argument to main.py.

jlewi · 2017-09-29T00:09:20Z

I think I responded to all your questions but let me know if I missed something.
Thanks for being persistent.

wbuchwalter · 2017-09-29T22:02:26Z

Hopefully it's starting to look better this time 😬
I still have to add unit tests and a few other small things, but some questions:

I added the path to grpc_tensorflow_server.py. as part of the ControllerConfig and not as a separate flag, is that okay with you?
Since this is part of the config, that means we now have to pass the controllerconfig down quite a few levels, should TrainingJob own a reference to the ControllerConfig instead?

Thanks for your help getting this right.

jlewi · 2017-10-01T23:05:33Z

Sorry for the slow reply.

I added the path to grpc_tensorflow_server.py. as part of the ControllerConfig and not as a separate flag, is that okay with you?

Seems reasonable to me.

Since this is part of the config, that means we now have to pass the controllerconfig down quite a few levels, should TrainingJob own a reference to the ControllerConfig instead?

Seems reasonable as well.

wbuchwalter · 2017-10-17T20:37:02Z

@jlewi Ready for review.

I also made two changes to the build_and_push.py script:

fix an issue with encoding (output of the command is bytes not string)
add a --no-push arg

jlewi

Thanks.

Please consider filing an issue to add an E2E test to cover the case where we use default PS.

jlewi · 2017-10-17T23:44:13Z

images/tf_operator/build_and_push.py

@@ -51,6 +53,8 @@ def run(command, cwd=None):
                      help="Use Google Container Builder to build the image.")
  parser.add_argument("--no-gcb", dest="use_gcb", action="store_false",
                      help="Use Docker to build the image.")
+  parser.add_argument("--no-push", dest="should_push", action="store_false",
+                      help="Push the image once build is finished.")


Don't you also need to define a "--push" argument with action "store_push"? and then do
parser.set_defaults(should_push=True)?

The help string for --no-push also looks incorrect.

Can you also pull in the latest changes that I just committed and resolve the conflict?

ArgParse automatically initialise the flag to the opposite value of the existing argument (so in this case true). I can still add --push if you want this to be explicit.

jlewi · 2017-10-17T23:46:36Z

pkg/controller/controller.go

@@ -147,7 +148,7 @@ func (c *Controller) handleTfJobEvent(event *Event) error {
 		//NewJob(kubeCli kubernetes.Interface, job spec.TfJob, stopC <-chan struct{}, wg *sync.WaitGroup)

 		c.stopChMap[clus.Metadata.Name] = stopC
-		c.jobs[clus.Metadata.Namespace + "-" + clus.Metadata.Name] = nc
+		c.jobs[clus.Metadata.Namespace+"-"+clus.Metadata.Name] = nc


nit: I think there should be spaces after the plus signs. Maybe run gofmt?

If I add this spaces back and run gofmt, it will remove them.
I am running go 1.9.1, could you have a different go version with different gofmt rules?

jlewi · 2017-10-17T23:47:44Z

pkg/spec/controller.go

@@ -5,6 +5,9 @@ type ControllerConfig struct {
 	// This should match the value specified as a container limit.
 	// e.g. alpha.kubernetes.io/nvidia-gpu
 	Accelerators map[string]AcceleratorConfig
+
+	// Path to the file containing the grpc server sources


Nit "sources" -> "source"

jlewi · 2017-10-17T23:53:36Z

pkg/spec/tf_job.go

@@ -75,7 +79,8 @@ const (
 type ContainerName string

 const (
-	TENSORFLOW ContainerName = "tensorflow"
+	TENSORFLOW     ContainerName = "tensorflow"
+	DefaultTFImage               = "tensorflow/tensorflow:latest"


Should we pin the default image to a particular version of TF? e.g. 1.2, 1.3 etc...? We can upgrade the default when we release a new TfJob CRD.

My concern is that TensorFlow introduces a lot of breaking changes with each TF version. So if people are relying on the default TF version supplied by the CRD, their jobs could start breaking suddenly when TF releases a new version.

jlewi · 2017-10-18T00:09:16Z

pkg/trainer/replicas.go

+
+	var buf bytes.Buffer
+	isFirstJob := true
+	for _, k := range keys {


nit: I'd prefer this code to be written as a series of joins so we don't have to have statements dealing with first character

so something like (without the syntax errors)

jobs = []string{} for _, jobType := range keys { hosts := []string{} for _, h := range clusterSpec[jobType] { hosts = append(hosts, h) } s = jobType + "|" + strings.join(hosts, ";") jobs = append(jobs, s) } return strings.join(jobs, ",")

The code might not generate the correct result but hopefully it illustrates what I mean by using strings.join

jlewi · 2017-10-18T00:13:16Z

pkg/trainer/replicas.go

+			log.Errorf("Error building PS ConfigMap: %v", err)
+			return err
+		}
+		_, err = s.ClientSet.CoreV1().ConfigMaps(NAMESPACE).Create(cm)


Should we be using the namespace of the job as opposed to the constant NAMESPACE?
I think #39 added support for creating TfJobs in other namespaces.
It looks like all the other resources use s.Job.job.Metadata.Namespace for the namespace

Can you delete the constant "NAMESPACE" in training.go since it should be unused?

jlewi · 2017-10-18T00:17:13Z

pkg/trainer/replicas.go

@@ -237,6 +333,16 @@ func (s *TFReplicaSet) Delete() error {
 		}
 	}

+	// If the ConfigMap for the default parameter server exists, we delete it
+	_, err = s.ClientSet.CoreV1().ConfigMaps(NAMESPACE).Get(s.defaultPSConfigMapName(), meta_v1.GetOptions{})


Update NAMESPACE as mentioned above.

wbuchwalter · 2017-10-18T21:35:06Z

@jlewi Should be good for another round.

jlewi · 2017-10-18T22:47:10Z

/test all

jlewi · 2017-10-19T00:38:15Z

Reviewed 1 of 8 files at r1, 4 of 14 files at r4, 3 of 11 files at r5, 1 of 1 files at r6, 2 of 4 files at r7.
Review status: 11 of 16 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

images/tf_operator/build_and_push.py, line 57 at r5 (raw file):

Previously, wbuchwalter (William Buchwalter) wrote…

ArgParse automatically initialise the flag to the opposite value of the existing argument (so in this case true). I can still add --push if you want this to be explicit.

Great thanks.

pkg/controller/controller.go, line 151 at r5 (raw file):

Previously, wbuchwalter (William Buchwalter) wrote…

If I add this spaces back and run gofmt, it will remove them.
I am running go 1.9.1, could you have a different go version with different gofmt rules?

Thanks. I filed a PR to set up lint checks as part of our testing.

Comments from Reviewable

jlewi · 2017-10-19T00:39:43Z

LGTM. I'm going to try to fix the test though before I merge it.

jlewi · 2017-10-19T01:21:17Z

/test all

jlewi · 2017-10-19T02:45:55Z

/test all

jlewi · 2017-10-19T02:56:34Z

/test all

jlewi · 2017-10-19T03:11:06Z

images/tf_operator/build_and_push.py

@@ -112,6 +118,8 @@ def run(command, cwd=None):
  else:
    run(["docker", "build", "-t", image,  context_dir])
    logging.info("Built image: %s", image)
+
+  if args.should_push:


The if args.should_push statement should be inside the else block. We only push the image if we aren't using GCB.

wbuchwalter · 2017-10-19T14:04:55Z

/retest

k8s-ci-robot · 2017-10-19T14:05:09Z

@wbuchwalter: you can't request testing unless you are a kubernetes member.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

jlewi · 2017-10-19T18:09:16Z

/test

jlewi · 2017-10-19T18:09:21Z

/test all

jlewi · 2017-10-19T18:27:50Z

Review status: 10 of 16 files reviewed at latest revision, 5 unresolved discussions.

Comments from Reviewable

jlewi · 2017-10-19T18:38:29Z

Woo Hoo!
Thanks @wbuchwalter

wbuchwalter added 3 commits September 11, 2017 11:05

implement default PS image

95ddfa8

Add default PS PodSpec

69ac7cc

add tests

c9f2d66

wbuchwalter assigned jlewi Sep 12, 2017

jlewi reviewed Sep 13, 2017

View reviewed changes

PS: Use ConfigMap instead of custom image

de8f8f4

jlewi reviewed Sep 14, 2017

View reviewed changes

move default PS clusterspec transform in Create

8082db3

jlewi reviewed Sep 26, 2017

View reviewed changes

wbuchwalter added 3 commits September 29, 2017 11:27

implement reviewer comments

7fa80cb

update docker image

8b233ef

make path to grpc server part of config

d5907df

wbuchwalter added 6 commits October 17, 2017 13:14

Merge branch 'master' of https://github.com/jlewi/mlkube.io into auto-ps

3f4f176

fix and add unit tests

c104f0e

fix and add unit tests

188e5cd

remove build_and_push.sh

c6f047d

fix build_and_push

7176834

Merge branch 'auto-ps' of github.com:wbuchwalter/mlkube.io into auto-ps

c8f00b5

jlewi reviewed Oct 18, 2017

View reviewed changes

wbuchwalter added 2 commits October 18, 2017 15:49

Merge branch 'master' of https://github.com/jlewi/mlkube.io into auto-ps

9f24996

implement reviewer comments

9a62ee9

jlewi mentioned this pull request Oct 18, 2017

presubmit test(bootstrap.py) doesn't properly check out PRs #59

Closed

jlewi suggested changes Oct 19, 2017

View reviewed changes

wbuchwalter added 2 commits October 19, 2017 09:30

Do not push with GCB

2c32e0b

Merge remote-tracking branch 'origin' into auto-ps

bb3fa53

kubeflow deleted a comment from k8s-ci-robot Oct 19, 2017

jlewi approved these changes Oct 19, 2017

View reviewed changes

jlewi merged commit ca33d3d into kubeflow:master Oct 19, 2017

jlewi mentioned this pull request Oct 19, 2017

Run TensorFlow server for parameter servers by default #16

Closed

		@@ -0,0 +1,22 @@
		# A very simple parameter server that joins the server defined by the cluster spec passed as environment variable

Parameter Server: Run TF server by default #36

Parameter Server: Run TF server by default #36

Conversation

wbuchwalter commented Sep 12, 2017 • edited by jlewi Loading

jlewi 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

wbuchwalter commented Sep 13, 2017 • edited Loading

jlewi commented Sep 13, 2017

wbuchwalter commented Sep 14, 2017

jlewi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbuchwalter commented Sep 15, 2017 • edited Loading

jlewi commented Sep 15, 2017

wbuchwalter commented Sep 21, 2017

jlewi commented Sep 22, 2017

wbuchwalter commented Sep 26, 2017

jlewi 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

wbuchwalter Sep 28, 2017 • edited Loading

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

jlewi commented Sep 29, 2017

wbuchwalter commented Sep 29, 2017 • edited Loading

jlewi commented Oct 1, 2017

wbuchwalter commented Oct 17, 2017

jlewi 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbuchwalter commented Oct 18, 2017

jlewi commented Oct 18, 2017

jlewi commented Oct 19, 2017

jlewi commented Oct 19, 2017

jlewi commented Oct 19, 2017

jlewi commented Oct 19, 2017

jlewi commented Oct 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbuchwalter commented Oct 19, 2017

k8s-ci-robot commented Oct 19, 2017

jlewi commented Oct 19, 2017

jlewi commented Oct 19, 2017

jlewi commented Oct 19, 2017

jlewi commented Oct 19, 2017

wbuchwalter commented Sep 12, 2017 •

edited by jlewi

Loading

wbuchwalter commented Sep 13, 2017 •

edited

Loading

wbuchwalter commented Sep 15, 2017 •

edited

Loading

wbuchwalter Sep 28, 2017 •

edited

Loading

wbuchwalter commented Sep 29, 2017 •

edited

Loading