Refactor the TfJob to use K8s libraries #215

wackxu · 2017-12-12T14:55:45Z

1.refactor the spec package to the kubernetes standard API, all detention of our CRD is in pkg pkg/apis/tensorflow and also the validation、 set Default for the CRD and some help func.

2.for use K8s code gen, wo import the project https://github.com/kubernetes/code-generator, and some shell script in the folder hack. You can see how to use it in https://blog.openshift.com/kubernetes-deep-dive-code-generation-customresources/

3.All the file in pkg/client are all auto generated and we should not modify it manually.

4.I do not plan to change the logical to use the sharedInformer and clientset. This PR is big enough, I think I can do it in a follow PR.

5.SharedInformer is a helpful tool with cache that we can use it to list and watch our resources. More details you can see http://borismattijssen.github.io/articles/kubernetes-informers-controllers-reflectors-stores

This change is

googlebot · 2017-12-12T14:55:48Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

k8s-ci-robot · 2017-12-12T14:55:52Z

Hi @wackxu. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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.

wackxu · 2017-12-12T14:57:49Z

I signed it!

jlewi · 2017-12-12T15:36:02Z

/ok-to-test

jlewi · 2017-12-12T18:24:50Z

Thanks for this PR. This looks like a great PR.

The PR is very big and I'm still trying to wrap my head around it. It would be very helpful if you could update the initial comment to provide more description about the motivation for the PR and the relevant changes. Specifically, it would be great if the initial comment in the PR could address the following questions.

Explain that we are using K8s code gen to generate the client.
- Which files are auto-generated?
Summarize the changes needed to support using K8s code gen
Explain the new package layout?
What is an informer and why/how you want to use them?

Reviewed 6 of 35 files at r1.
Review status: 6 of 35 files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

hack/update-codegen.sh, line 2 at r1 (raw file):

#!/bin/bash

Please add a comment explaining what this is for?

hack/update-codegen.sh, line 28 at r1 (raw file):

#                  k8s.io/kubernetes. The output-base is needed for the generators to output into the vendor dir
#                  instead of the $GOPATH directly. For normal projects this can be dropped.
${CODEGEN_PKG}/generate-groups.sh "deepcopy,client,informer,lister" \

Can these scripts for code gen be moved into their own PR to reduce the PR size?

hack/verify-codegen.sh, line 2 at r1 (raw file):

#!/bin/bash

Please add a comment explaining what this is for.

pkg/apis/tensorflow/helper/helpers.go, line 1 at r1 (raw file):

package helper

Is there a package level comment somewhere?

pkg/apis/tensorflow/helper/helpers.go, line 12 at r1 (raw file):

)

func AsOwner(tfJob *tfv1.TfJob) metav1.OwnerReference {

Exported functions should have comments.

pkg/apis/tensorflow/helper/helpers.go, line 14 at r1 (raw file):

func AsOwner(tfJob *tfv1.TfJob) metav1.OwnerReference {
	trueVar := true
	// TODO: In 1.6 this is gonna be "k8s.io/kubernetes/pkg/apis/meta/v1"

I think we can delete this TODO since we require 1.8 for CRD.

pkg/apis/tensorflow/helper/helpers.go, line 83 at r1 (raw file):

}

// Cleanup cleans up user passed spec, e.g. defaulting, transforming fields.

Is the convention to use "Cleanup" to denote this functionality?
All TODO's should have a name attached to them. The name should be the person with the most context about the requested change. Usually this will be the author of the TODO.

pkg/apis/tensorflow/helper/helpers.go, line 95 at r1 (raw file):

// TODO(jlewi): Get rid of the append methods that we don't need
func AppendScalingDownCondition(cs *tfv1.TfJobStatus, from, to int) {

I think we can delete this since we don't have a scaling down condition.

pkg/apis/tensorflow/helper/helpers.go, line 104 at r1 (raw file):

}

func AppendRecoveringCondition(cs *tfv1.TfJobStatus) {

What is a recovering condition? Can you add a comment since its an exported method?

pkg/apis/tensorflow/helper/helpers.go, line 112 at r1 (raw file):

}

func AppendUpgradingCondition(cs *tfv1.TfJobStatus, to string, member string) {

Here and below, please add comments explaining what these conditions mean in terms of the TfJob semantics.

pkg/apis/tensorflow/helper/helpers.go, line 123 at r1 (raw file):

}

func AppendRemovingDeadMember(cs *tfv1.TfJobStatus, name string) {

What is this condition in the context of TfJob?

pkg/apis/tensorflow/helper/helpers.go, line 159 at r1 (raw file):

}

func scalingReason(from, to int) string {

Do we need this? TfJob currently doesn't support scaling.

pkg/apis/tensorflow/v1alpha1/defaults.go, line 1 at r1 (raw file):

package v1alpha1

Package comment.

pkg/apis/tensorflow/v1alpha1/defaults.go, line 11 at r1 (raw file):

// SetDefaults sets any unspecified values to defaults
func SetDefaults_TfJobSpec(c *TfJobSpec) error {

Why did you make SetDefaults a pure function as opposed to making at a member function (I know that's not the correct terminalogy) of TfJobSpec?

pkg/apis/tensorflow/v1alpha1/defaults.go, line 35 at r1 (raw file):

		//Set the default configuration for a PS server if the user didn't specify a PodTemplateSpec
		if r.Template == nil && r.TfReplicaType == PS {

The code to sSetDefaults changed in http://github.com/tensorflow/k8s/pull/204 . That code started setting the TerminationPolicy. Can you update the code please?

Comments from Reviewable

jlewi · 2017-12-12T22:13:38Z

Ignore my previous comment about splitting this PR up. I looked at it more thoroughly and it looks like the bulk of it is autogenerated code I think its fine to submit this as one PR.

Overall the code looks good. I think my main comments are to delete some of the code that is a legacy of the etcd operator that we don't need anymore.

Reviewed 26 of 35 files at r1.
Review status: 32 of 35 files reviewed at latest revision, 23 unresolved discussions, some commit checks failed.

pkg/apis/tensorflow/v1alpha1/defaults.go, line 1 at r1 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Package comment.

NM. There's a comment in doc.go.

pkg/apis/tensorflow/v1alpha1/types.go, line 20 at r1 (raw file):

)

// +genclient

Are these annotations that the code gen client understands?

pkg/apis/tensorflow/v1alpha1/types.go, line 33 at r1 (raw file):

}

// TODO(jlewi): Need to define the actual configuration for the TensorFlow TfJob.

I think we can delete this TODO now.

pkg/apis/tensorflow/v1alpha1/types.go, line 112 at r1 (raw file):

// TODO(jlewi): Need to define appropriate conditions and get rid of the ones we don't need.
const (
	TfJobConditionReady              = "Ready"

Conditions were copied from the etcd cluster. I think we should delete them.

pkg/apis/tensorflow/v1alpha1/types.go, line 136 at r1 (raw file):

	// ControlPuased indicates the operator pauses the control of the cluster.
	// TODO(jlewi): I think we can get rid of ControlPaued.
	ControlPaused bool `json:"controlPaused"`

I think we can delete ControlPaused. Currently there's no notion of pausing a job. If we want to add that later we can add it back.

pkg/apis/tensorflow/v1alpha1/types.go, line 139 at r1 (raw file):

	// Condition keeps ten most recent cluster conditions
	Conditions []TfJobCondition `json:"conditions"`

I think we should delete conditions. It was inherited from etcd operator and I don't think it makes sense for TfJob.

pkg/apis/tensorflow/v1alpha1/types.go, line 171 at r1 (raw file):

// +resource:path=tfjobs

// TfJobList is a list of etcd clusters.

etcd -> TfJobs

pkg/apis/tensorflow/v1alpha1/types.go, line 177 at r1 (raw file):

	// More info: http://releases.k8s.io/HEAD/docs/devel/api-conventions.md#metadata
	Metadata metav1.ListMeta `json:"metadata,omitempty"`
	// Items is a list of third party objects

third party objects -> TfJobs

pkg/apis/tensorflow/validation/validation.go, line 31 at r1 (raw file):

		isValidReplicaType := false
		for _, t := range validReplicaTypes {

Can you add the validation that was added in #204 to check termination policy?

pkg/client/clientset/versioned/typed/tensorflow/v1alpha1/tfjob.go, line 17 at r1 (raw file):

*/

package v1alpha1

This file is autogenerated right even though it doesn't have a comment to that effect?

Comments from Reviewable

gaocegege · 2017-12-13T02:48:02Z

Reviewed 35 of 35 files at r1.
Review status: all files reviewed at latest revision, 25 unresolved discussions, some commit checks failed.

glide.yaml, line 19 at r1 (raw file):

github.com/docker/distribution
Do we need this dependency?

pkg/apis/tensorflow/v1alpha1/register.go, line 33 at r1 (raw file):

  metav1.AddToGroupVersion(scheme, SchemeGroupVersion)
  return nil
}

It is better to add a new line.

pkg/apis/tensorflow/v1alpha1/types.go, line 20 at r1 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Are these annotations that the code gen client understands?

Yeah, we have to add these annotations to make code-generator works.

Comments from Reviewable

jlewi · 2017-12-13T20:31:26Z

Let me know when this is ready for another look.

wackxu · 2017-12-14T02:48:00Z

@jlewi Sorry for the delay, I am very busy now and today night I will fix all the comments.

wackxu · 2017-12-14T02:48:29Z

When I fix it, I will call you @jlewi

wackxu · 2017-12-14T15:04:20Z

@jlewi Just update the PR comment and sorry that I will addressed the comments tomorrow, It is about eleven twenty in the evening and I need to go to home.

jlewi · 2017-12-14T19:55:17Z

No rush on my end. Just want to make sure you aren't blocked waiting on me.

Thank you very much for doing this.

* update k8s dependency to stable version * now the dependency for k8s is not the release version and this causes problems for #215, because the code-generator is outdated but the latest version for kubernetes 1.8.X fixes problem.

wackxu · 2017-12-19T14:54:49Z

@jlewi Addressed the comments, Can you take a look? I will refactor the logic in the PR #234, I think the change is big, we should have a detail look.

jlewi · 2017-12-20T03:56:09Z

Reviewed 31 of 527 files at r2.
Review status: 55 of 551 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

hack/update-codegen.sh, line 2 at r1 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Please add a comment explaining what this is for?

Can you add a comment please?

hack/update-codegen.sh, line 28 at r1 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Can these scripts for code gen be moved into their own PR to reduce the PR size?

As I mentioned elsewhere I think it makes sense to keep them in this PR.

pkg/apis/tensorflow/helper/helpers.go, line 83 at r1 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Is the convention to use "Cleanup" to denote this functionality?
All TODO's should have a name attached to them. The name should be the person with the most context about the requested change. Usually this will be the author of the TODO.

I think we can take care of cleaning this up in a follow on PR since this PR didn't actually add the cleanup function.

pkg/apis/tensorflow/helper/helpers.go, line 159 at r1 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Do we need this? TfJob currently doesn't support scaling.

OK

pkg/apis/tensorflow/v1alpha1/defaults.go, line 11 at r1 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Why did you make SetDefaults a pure function as opposed to making at a member function (I know that's not the correct terminalogy) of TfJobSpec?

I think you missed this question?

pkg/apis/tensorflow/v1alpha1/defaults.go, line 35 at r1 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

The code to sSetDefaults changed in http://github.com/tensorflow/k8s/pull/204 . That code started setting the TerminationPolicy. Can you update the code please?

ok

pkg/apis/tensorflow/v1alpha1/register.go, line 33 at r1 (raw file):

Previously, gaocegege (Ce Gao) wrote…

It is better to add a new line.

Can you add a newline please?

Comments from Reviewable

jlewi · 2017-12-20T03:57:45Z

There were a couple unresolved comments but nothing major. All the vendor changes makes reviewing this PR a pain. So I'm going to merge and we can follow up in folllow on PRs.

Review status: 55 of 551 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

Comments from Reviewable

jlewi · 2017-12-20T04:01:55Z

Actually can you fix the travis build, it looks like there is an issue with dependencies

$ go test -v ./pkg/...

vendor/k8s.io/apimachinery/pkg/util/cache/lruexpirecache.go:23:2: cannot find package "github.com/hashicorp/golang-lru" in any of:

	/home/travis/gopath/src/github.com/tensorflow/k8s/vendor/github.com/hashicorp/golang-lru (vendor tree)

	/home/travis/.gimme/versions/go1.8.5.linux.amd64/src/github.com/hashicorp/golang-lru (from $GOROOT)

	/home/travis/gopath/src/github.com/hashicorp/golang-lru (from $GOPATH)

I suspect since we check in the vendor directory we no longer need to run glide install and that might fix the issue.

jlewi · 2017-12-21T06:03:06Z

I think #236 (removing glide install from travis build) should also fix the issue.

ScorpioCPH

Thanks for your good refactor works!

This is a very big PR, so I only take a look at API definition, and leave some quick comments here:

There are some default value but i think it is defined by user.
Some state is a little complicated, maybe it is difficult to maintain so many states in the control logic.

ScorpioCPH · 2017-12-24T13:15:02Z

pkg/apis/tensorflow/v1alpha1/types.go

+type AcceleratorVolume struct {
+	Name      string
+	HostPath  string
+	MountPath string


Just FYI, there is no need to mount GPU manually if we use nvidia device-plugin and nvidia-docker.

Some useful information here: kubernetes/kubernetes#54011

Do all Kubernetes distributions support device-plugin? As of a couple months ago I still think we needed to use the file path on ACS and some other platforms.

Yes, I think device-plugin will be beta (enabled by default) in 1.10, and NVIDIA GPU discovery feature (alpha.kubernetes.io/nvidia-gpu) will be deprecated in the future.

Please read this doc for more details: kubernetes/website#6736.

ScorpioCPH · 2017-12-24T13:16:51Z

pkg/apis/tensorflow/v1alpha1/defaults.go

+// SetDefaults sets any unspecified values to defaults
+func SetDefaults_TfJobSpec(c *TfJobSpec) error {
+	if c.TfImage == "" {
+		c.TfImage = DefaultTFImage


Is this TfImage required, why should we set a default value for this?

ScorpioCPH · 2017-12-24T13:21:13Z

pkg/apis/tensorflow/v1alpha1/defaults.go

+
+		//Set the default configuration for a PS server if the user didn't specify a PodTemplateSpec
+		if r.Template == nil && r.TfReplicaType == PS {
+			setDefaultPSPodTemplateSpec(r, c.TfImage)


Why should we set a default value here?

ScorpioCPH · 2017-12-24T13:21:30Z

pkg/apis/tensorflow/v1alpha1/defaults.go

+		}
+
+		if string(r.TfReplicaType) == "" {
+			r.TfReplicaType = MASTER


IMHO, should this be required? and MASTER is only one in distributed TF training.

@wackxu this should fix

ScorpioCPH · 2017-12-24T13:23:09Z

pkg/apis/tensorflow/v1alpha1/types.go

+
+type TfJobSpec struct {
+	// TODO(jlewi): Can we we get rid of this and use some value from Kubernetes or a random ide.
+	RuntimeId string


Can we reuse metadata.UID?

Can you open up an issue for this?

ScorpioCPH · 2017-12-24T13:26:05Z

pkg/apis/tensorflow/v1alpha1/types.go

+	State ReplicaState `json:"state"`
+
+	// ReplicasStates provides the number of replicas in each status.
+	ReplicasStates map[ReplicaState]int


Nit: why there is so many states, State/ReplicaState/TfReplicaStatus

Can you open up an issue for this?

jlewi · 2017-12-25T18:19:54Z

/test all

jlewi · 2017-12-25T18:23:46Z

@ScorpioCPH Can you open up an issue with your comments about the API issues? This PR is primarily changing the existing implementation but not the API. So it would be better to address API changes in another issue.

There are already a some API related issues tagged with the API label.

jlewi · 2017-12-26T22:41:00Z

Any idea what's going on with the test failures?

Maybe try pulling in the latest changes from master. It looks like the travis build is still running glide install but that should have been fixed by #236.

@wackxu I think we can merge this as soon as the test failures are fixed. Would be great to get this merged since it touches a lot of files and will unblock other refactoring.

wackxu · 2017-12-27T13:26:05Z

Why did you make SetDefaults a pure function as opposed to making at a member function (I know that's not the correct terminalogy) of TfJobSpec?

@jlewi This style is keep to be consistent with k8s. And I have add auto generated defaulter to use the SetDefaults function. It is referenced by auto generated filegenerated zz_generated.defaulter.go

wackxu · 2017-12-27T13:33:25Z

Sorry for the delay, Addressed comments. PTAL. I will do all the refactor work in follow two days since I am not busy now.

zjj2wry

leaves some comments，this PR generated lgtm. @wackxu thank you do this ：）

zjj2wry · 2017-12-27T15:09:47Z

pkg/apis/tensorflow/v1alpha1/defaults.go

+		}
+	}
+
+	return nil


should not return nil just return

zjj2wry · 2017-12-27T15:13:03Z

pkg/apis/tensorflow/v1alpha1/defaults.go

+	return nil
+}
+
+func setDefault_PSPodTemplateSpec(r *TfReplicaSpec, tfImage string) {


seems this func not join defaulter generator, we can remove this？

@zjj2wry The setDefault_PSPodTemplateSpec is called by the SetDefaults_TfJob func. I think we should keep it.

you are right, i miss this~

zjj2wry · 2017-12-27T15:13:22Z

pkg/apis/tensorflow/v1alpha1/defaults.go

+}
+
+
+// SetDefaults sets any unspecified values to defaults


fix this lints

Can you attach more info about lints errors :)
It maybe helpful to fix this.

zjj2wry · 2017-12-27T15:33:40Z

@wackxu maybe loses some dependency~
@jlewi @ScorpioCPH this PR in order to add k8s client package, i suggest merge this when wackxu fix defaulter comments. And we should fix api convention in other PR.

1.11s$ $GOPATH/bin/goveralls -service=travis-ci -v -package ./pkg/...
exit status 1: vendor/k8s.io/apimachinery/pkg/util/cache/lruexpirecache.go:23:2: cannot find package "github.com/hashicorp/golang-lru" in any of:
	/home/travis/gopath/src/github.com/tensorflow/k8s/vendor/github.com/hashicorp/golang-lru (vendor tree)
	/home/travis/.gimme/versions/go1.8.linux.amd64/src/github.com/hashicorp/golang-lru (from $GOROOT)
	/home/travis/gopath/src/github.com/hashicorp/golang-lru (from $GOPATH)

jlewi · 2017-12-27T18:01:29Z

@zjj2wry Seems reasonable to me. I think we can merge this as soon as the tests are fixed and fix any remaining issues in follow up PRs.

wackxu · 2017-12-28T08:28:09Z

@jlewi Addressed comments, PTLA

jlewi · 2017-12-28T14:13:28Z

Reviewed 1 of 527 files at r2, 1 of 6 files at r3, 1 of 1 files at r4.
Review status: 55 of 552 files reviewed at latest revision, 13 unresolved discussions, some commit checks failed.

Comments from Reviewable

jlewi · 2017-12-28T14:14:24Z

Any idea why the travis build is still failing? Not sure why build would succeed in the presubmit but not the travis build.

$ $GOPATH/bin/goveralls -service=travis-ci -v -package ./pkg/...
exit status 1: vendor/k8s.io/apimachinery/pkg/util/cache/lruexpirecache.go:23:2: cannot find package "github.com/hashicorp/golang-lru" in any of:
	/home/travis/gopath/src/github.com/tensorflow/k8s/vendor/github.com/hashicorp/golang-lru (vendor tree)
	/home/travis/.gimme/versions/go1.9.linux.amd64/src/github.com/hashicorp/golang-lru (from $GOROOT)
	/home/travis/gopath/src/github.com/hashicorp/golang-lru (from $GOPATH)

coveralls · 2017-12-29T02:12:50Z

Coverage decreased (-9.2%) to 28.548% when pulling fa7ac01 on wackxu:refk8s into 420f9aa on tensorflow:master.

wackxu · 2017-12-29T02:27:11Z

@jlewi fix travis build fail， PTAL

zjj2wry

LGTM

zjj2wry · 2017-12-29T07:26:13Z

travis-ci fixed, let's merge this unblock other refactor.

jlewi · 2017-12-29T16:50:23Z

Thanks for the hard work.

k8s-ci-robot added the needs-ok-to-test label Dec 12, 2017

k8s-ci-robot removed the needs-ok-to-test label Dec 12, 2017

gaocegege mentioned this pull request Dec 14, 2017

Use conditions instead of phase #223

Closed

wackxu mentioned this pull request Dec 17, 2017

update k8s dependency to stable version #230

Merged

wackxu mentioned this pull request Dec 19, 2017

refactor the controller logic #234

Merged

jlewi changed the title ~~[WIP] refactor the TfJob to use K8s libraries~~ Refactor the TfJob to use K8s libraries Dec 20, 2017

ScorpioCPH reviewed Dec 24, 2017

View reviewed changes

ScorpioCPH mentioned this pull request Dec 27, 2017

API: some comments about API changes from PR #215 review #249

Closed

zjj2wry approved these changes Dec 27, 2017

View reviewed changes

wackxu added 5 commits December 29, 2017 10:03

refactor the spec to the standard k8s API group

fb65942

add code-generater shell script

1edafed

add code-generator dependency

5ad4773

add generated file

32f31b9

address travis build fail

fa7ac01

zjj2wry approved these changes Dec 29, 2017

View reviewed changes

jlewi approved these changes Dec 29, 2017

View reviewed changes

jlewi merged commit ed7cae7 into kubeflow:master Dec 29, 2017

jlewi mentioned this pull request Dec 29, 2017

Build failing because of lint issues #256

Closed

Refactor the TfJob to use K8s libraries #215

Refactor the TfJob to use K8s libraries #215

Conversation

wackxu commented Dec 12, 2017 • edited by jlewi Loading

googlebot commented Dec 12, 2017

k8s-ci-robot commented Dec 12, 2017

wackxu commented Dec 12, 2017

jlewi commented Dec 12, 2017

jlewi commented Dec 12, 2017

jlewi commented Dec 12, 2017

gaocegege commented Dec 13, 2017

jlewi commented Dec 13, 2017

wackxu commented Dec 14, 2017

wackxu commented Dec 14, 2017

wackxu commented Dec 14, 2017

jlewi commented Dec 14, 2017

wackxu commented Dec 19, 2017

jlewi commented Dec 20, 2017

jlewi commented Dec 20, 2017

jlewi commented Dec 20, 2017

jlewi commented Dec 21, 2017

ScorpioCPH 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

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 Dec 25, 2017

jlewi commented Dec 25, 2017

jlewi commented Dec 26, 2017

wackxu commented Dec 27, 2017

wackxu commented Dec 27, 2017

zjj2wry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zjj2wry Dec 27, 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

zjj2wry commented Dec 27, 2017 • edited Loading

jlewi commented Dec 27, 2017

wackxu commented Dec 28, 2017

jlewi commented Dec 28, 2017

jlewi commented Dec 28, 2017

coveralls commented Dec 29, 2017 • edited Loading

wackxu commented Dec 29, 2017

zjj2wry left a comment

Choose a reason for hiding this comment

zjj2wry commented Dec 29, 2017

jlewi commented Dec 29, 2017

wackxu commented Dec 12, 2017 •

edited by jlewi

Loading

zjj2wry Dec 27, 2017 •

edited

Loading

zjj2wry commented Dec 27, 2017 •

edited

Loading

coveralls commented Dec 29, 2017 •

edited

Loading