API Review about training-operator

[ScorpioCPH]

I also have some comments about API changes #249, maybe we can discuss this topic together :)

[gaocegege]

• Instead of specifying replica type should we specify features e.g. run forever, restart always, etc...

I think the type name tfReplicaType is a little redundant, maybe type is fine.

And I agree with @ScorpioCPH about the status:

Maybe we should revisit status representation: State/ReplicaState/TfReplicaStatus.

For example, I think State field in TFReplicaStatus could not express the state accurately since it is complicated, then TFReplicasStates is enough maybe.

[0xgj]

I suggest we should use chief instead of master as #306 stated

* it's confusing using master when there is distributed master in tensorflow;
* due to issue #61 , in tensorflow 1.4, TF_CONFIG use chief instead of master;

@jlewi @ScorpioCPH @gaocegege

[jlewi]

I agree we should probably get rid of "master". I actually think we should probably get rid of ReplicaType and introduce properties that control different behaviors of the replica such as termination policy. Users can then assign an arbitrary name to each replica.

[0xgj]

+1, @jlewi we can use a property named job or name instead of replicaType, which can be assigned to any valid string. other terms we can use include task, group... by default job will be set to worker.

apiVersion: "tensorflow.org/v1alpha1"
kind: "TfJob"
metadata:
  name: "example-job"
spec:
  replicaSpecs:
    - replicas: 1
      name: chief
      template:
        spec:
          containers:
            - image: gcr.io/tf-on-k8s-dogfood/tf_sample:dc944ff
              name: chief   ## if i need a seperate chief, add volumes if we want persistent logs
          restartPolicy: OnFailure
    - replicas: 1
      name: worker
      template:
        spec:
          containers:
            - image: gcr.io/tf-on-k8s-dogfood/tf_sample:dc944ff
              name: worker
          restartPolicy: OnFailure
    - replicas: 2
        name: ps
        template:
          spec:
            containers:
            - image: gcr.io/tf-on-k8s-dogfood/tf_sample:dc944ff
              name: ps
          restartPolicy: OnFailure

[ScorpioCPH]

I think the type name tfReplicaType is a little redundant, maybe type is fine.

It seems like we must use something to specify the Type of TFReplica.
+1 for type.

I don't think name or job can represent this exactly.

[jlewi]

Closing this issue since the proposal for the v1alpha2 API has been approved.