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

Deprecate IsDefaultPS in TFJob CRD API #343

Merged
merged 2 commits into from
Jan 25, 2018

Conversation

ScorpioCPH
Copy link
Member

@ScorpioCPH ScorpioCPH commented Jan 24, 2018

This PR fixes #329:

  • Remove IsDefaultPS in TFJob CRD API
  • Clean up hack/grpc_tensorflow_server/grpc_tensorflow_server.py which is useless
  • Update related docs

@jlewi @gaocegege @DjangoPeng PTAL.


This change is Reviewable

@coveralls
Copy link

coveralls commented Jan 24, 2018

Coverage Status

Coverage decreased (-0.6%) to 30.933% when pulling 7864dc8 on ScorpioCPH:deprecate-default-ps into 11b2fad on tensorflow:master.

@ScorpioCPH
Copy link
Member Author

/test tf-k8s-presubmit

@jlewi
Copy link
Contributor

jlewi commented Jan 24, 2018

Anybody else want to take a look?

@wbuchwalter
Copy link
Contributor

wbuchwalter commented Jan 24, 2018

setDefault_PSPodTemplateSpec is still present in pkg/apis/tensorflow/v1aplha1/defaults.go and not specifying a PodTemplateSpec for the PS is still valid (see`https://github.com/ScorpioCPH/k8s/blob/deprecate-default-ps/pkg/apis/tensorflow/validation/validation.go#L22).

So this would mean that if the user does not specify a custom image for the PS, there would be no error thrown, and the default command for TensorFlow's base image would execute at runtime, so it would start a Jupyter notebook IIRC, since we don't have grpc_tensorflow_server.py anymore.

Did I miss something?

If not, we should entirely remove the logic for default PS and force the user to specify a PodTemplateSpec for PS as well.

@ScorpioCPH
Copy link
Member Author

@wbuchwalter Thanks! Addressed your comments, PTAL.

@wbuchwalter wbuchwalter self-requested a review January 25, 2018 02:25
@ScorpioCPH ScorpioCPH force-pushed the deprecate-default-ps branch from cb1534c to 7864dc8 Compare January 25, 2018 02:47
@jlewi jlewi merged commit 296b652 into kubeflow:master Jan 25, 2018
@ScorpioCPH ScorpioCPH deleted the deprecate-default-ps branch January 26, 2018 02:36
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.

Deprecate the IsDefaultPS field
4 participants