Skip to content

Rename Tf to TF #332

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jan 23, 2018
Merged

Rename Tf to TF #332

merged 2 commits into from
Jan 23, 2018

Conversation

@ScorpioCPH
Copy link
Member

@ScorpioCPH ScorpioCPH commented Jan 19, 2018 •
edited by jlewi
Loading

As we discuss here #328

Rename Tf to TF to make more conventional. Fixes #328.

@jlewi PTAL.

This change is Reviewable

@coveralls
Copy link

coveralls commented Jan 19, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.1%) to 31.541% when pulling 8396d14 on ScorpioCPH:rename-TfJob-to-TFJob into 04425d9 on tensorflow:master.

@gaocegege
Copy link
Member

gaocegege commented Jan 19, 2018

And, I think the doc should be updated, too.

@DjangoPeng
Copy link
Member

DjangoPeng commented Jan 19, 2018

Good job! @ScorpioCPH

I agree with @gaocegege and we'd better update the docs in the same PR to keep the unity.

@coveralls
Copy link

coveralls commented Jan 19, 2018 •
edited
Loading

Coverage Status

Coverage remained the same at 31.439% when pulling 854e496 on ScorpioCPH:rename-TfJob-to-TFJob into 04425d9 on tensorflow:master.

@ScorpioCPH
Copy link
Member Author

ScorpioCPH commented Jan 19, 2018

@gaocegege @DjangoPeng Thanks, done.

@jlewi
Copy link
Contributor

jlewi commented Jan 19, 2018

Can you provide examples of K8s resources to show that is the convention in Kubernetes?

@ScorpioCPH
Copy link
Member Author

ScorpioCPH commented Jan 20, 2018

@jlewi Sure, this is coding-conventions and api-conventions.

For example here:

// Filesystem type of the volume that you want to mount.
// Tip: Ensure that the filesystem type is supported by the host operating system.
// Examples: "ext4", "xfs", "ntfs". Implicitly inferred to be "ext4" if unspecified.
// More info: https://kubernetes.io/docs/concepts/storage/volumes#rbd
// TODO: how do we prevent errors in the filesystem from compromising the machine
// +optional
FSType string `json:"fsType,omitempty" protobuf:"bytes,3,opt,name=fsType"`

We chose FSType instead of FsType :)

@ScorpioCPH ScorpioCPH force-pushed the rename-TfJob-to-TFJob branch from 854e496 to 1938067 Compare January 20, 2018 14:50
@coveralls
Copy link

coveralls commented Jan 20, 2018 •
edited
Loading

Coverage Status

Coverage remained the same at 31.611% when pulling d6a9510 on ScorpioCPH:rename-TfJob-to-TFJob into ca638ed on tensorflow:master.

@jlewi
Copy link
Contributor

jlewi commented Jan 20, 2018

My take on K8s API here is that they use the generally accepted capitalization e.g.

  • SSHAuthPrivateKey
  • SecretTypeTLS
  • TLSCertKey
  • TLSPrivateKeyKey
  • NFS

I'm convinced that TFJob is better.

Lets give people until Monday to chime in; I'll post to slack. I don't want to have to change it again.

@jlewi
Copy link
Contributor

jlewi commented Jan 20, 2018

/cc @wbuchwalter

@wbuchwalter
Copy link
Contributor

wbuchwalter commented Jan 22, 2018

+1 for TFJob as it is also standard in Golang, i.e: QuoteToASCII.

@ConnorDoyle
Copy link
Contributor

ConnorDoyle commented Jan 22, 2018

+1

@DjangoPeng
Copy link
Member

DjangoPeng commented Jan 23, 2018

@ScorpioCPH We have to rebase the PR.

@ScorpioCPH ScorpioCPH force-pushed the rename-TfJob-to-TFJob branch from 1938067 to d6a9510 Compare January 23, 2018 02:17
@ScorpioCPH
Copy link
Member Author

ScorpioCPH commented Jan 23, 2018

/test tf-k8s-presubmit

@ScorpioCPH
Copy link
Member Author

ScorpioCPH commented Jan 23, 2018

@jlewi PTAL, python -m py.release build seems ok on my local, have no idea about the tf-k8s-presubmit test failed.

@jlewi jlewi added this to the Kubecon Europe milestone Jan 23, 2018
@jlewi
Copy link
Contributor

jlewi commented Jan 23, 2018

Looks like the tests failed.

@jlewi
Copy link
Contributor

jlewi commented Jan 23, 2018

/test all

@jlewi
Copy link
Contributor

jlewi commented Jan 23, 2018

Looks like the test failure was because we ran out of disk space. let me clean it up.

@ScorpioCPH
Copy link
Member Author

ScorpioCPH commented Jan 23, 2018

/test tf-k8s-presubmit

@DjangoPeng DjangoPeng merged commit 74a958b into kubeflow:master Jan 23, 2018
@DjangoPeng
Copy link
Member

DjangoPeng commented Jan 23, 2018

Merged.

@jlewi We'd better start squash merging in the next PR.

@ScorpioCPH ScorpioCPH deleted the rename-TfJob-to-TFJob branch January 23, 2018 08:32
@jlewi
Copy link
Contributor

jlewi commented Jan 23, 2018

@DjangoPeng yes please always do squash merge.

@jlewi jlewi mentioned this pull request Jan 25, 2018
Closed
sanchitarora pushed a commit to sanchitarora/tensorflow-k8s-azure that referenced this pull request Jan 26, 2018
Tf* was renamed to TF* in the custom resource definition for k8s - ku…
d306924 
…beflow/trainer#332

This was causing an error `error: unable to recognize "config.yaml": no matches for tensorflow.org/, Kind=TfJob` where the Kind of job was not identified by following the README. Updated the reference so that people following the guide don't face the same trouble in the future.

Bunch of other spell corrects :)
@sanchitarora sanchitarora mentioned this pull request Jan 26, 2018
Merged
@gaocegege gaocegege mentioned this pull request Jan 30, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gaocegege gaocegege gaocegege approved these changes

+1 more reviewer

@jlewi jlewi jlewi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Kubecon Europe

Development

Successfully merging this pull request may close these issues.

7 participants

@ScorpioCPH @coveralls @gaocegege @DjangoPeng @jlewi @wbuchwalter @ConnorDoyle