Skip to content

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