Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix setting defaults. #299

Merged
merged 4 commits into from
Jan 12, 2018
Merged

Fix setting defaults. #299

merged 4 commits into from
Jan 12, 2018

Conversation

jlewi
Copy link
Contributor

@jlewi jlewi commented Jan 12, 2018


This change is Reviewable

* Need to register the default functions.
* In setup we need to invoke setting the defaults on the objects.
* This fixes a break introduced when we refactored the code.
@jlewi jlewi requested review from wackxu and a user January 12, 2018 00:49
@coveralls
Copy link

coveralls commented Jan 12, 2018

Coverage Status

Coverage increased (+0.8%) to 31.43% when pulling f087e23 on jlewi:fix_defaults into e619ac4 on tensorflow:master.

@gaocegege
Copy link
Member

The job in prow is weird: https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/tensorflow_k8s/299/tf-k8s-presubmit/354/


Result | SUCCESS
-- | --
Tests | 5 failed / 5 succeeded


@wackxu
Copy link
Contributor

wackxu commented Jan 12, 2018

@jlewi Sorry for the mistake and Thanks for your fix. Can we keep style with k8s and add function below:

func init() {
	// We only register manually written functions here. The registration of the
	// generated functions takes place in the generated files. The separation
	// makes the code compile even when the generated files are missing.
	SchemeBuilder.Register(addDefaultingFuncs)
}

@jlewi
Copy link
Contributor Author

jlewi commented Jan 12, 2018

@wackxu NP. What file is the init function in?

@wackxu
Copy link
Contributor

wackxu commented Jan 12, 2018

@jlewi The one you modifed pkg/apis/tensorflow/v1alpha1/register.go

@jlewi
Copy link
Contributor Author

jlewi commented Jan 12, 2018

@wackxu PTAL

@jlewi
Copy link
Contributor Author

jlewi commented Jan 12, 2018

@wackxu
Copy link
Contributor

wackxu commented Jan 12, 2018

/lgtm

@coveralls
Copy link

coveralls commented Jan 12, 2018

Coverage Status

Coverage increased (+1.0%) to 31.609% when pulling b51222a on jlewi:fix_defaults into e619ac4 on tensorflow:master.

@wackxu
Copy link
Contributor

wackxu commented Jan 12, 2018

@jlewi Seems there have an gometalinter error, just run goimports -w pkg/trainer/training.go

pkg/trainer/training.go:1::warning: file is not goimported (goimports)

// generated functions takes place in the generated files. The separation
// makes the code compile even when the generated files are missing.
SchemeBuilder.Register(addDefaultingFuncs)
}
Copy link
Member

@gaocegege gaocegege Jan 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add addKnownTypes here:

SchemeBuilder.Register(addKnownTypes)

And here https://github.com/tensorflow/k8s/pull/299/files#diff-2f7eaaeac6f6da3e4c93e8530a8a4e4aL10 should be:

SchemeBuilder      runtime.SchemeBuilder

I think it may be clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not think so, I think now is better and there is nothing that are confuse.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, then LGTM

@coveralls
Copy link

coveralls commented Jan 12, 2018

Coverage Status

Coverage increased (+1.0%) to 31.609% when pulling beee7f8 on jlewi:fix_defaults into e619ac4 on tensorflow:master.

@coveralls
Copy link

coveralls commented Jan 12, 2018

Coverage Status

Coverage increased (+1.09%) to 31.714% when pulling beee7f8 on jlewi:fix_defaults into e619ac4 on tensorflow:master.

@gaocegege
Copy link
Member

gaocegege commented Jan 12, 2018

And this PR also fixes #297

@jlewi jlewi merged commit 5cf32fa into kubeflow:master Jan 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fail to run example job. invalid job spec: tfReplicaSpec.TfPort can''t be nil
4 participants