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

Remove TensorBoard related code in operator #391

Merged
merged 4 commits into from
Feb 27, 2018
Merged

Remove TensorBoard related code in operator #391

merged 4 commits into from
Feb 27, 2018

Conversation

gaocegege
Copy link
Member

@gaocegege gaocegege commented Feb 14, 2018

Ref #347

Blocked until CI is running again.

PS: Dashboard code is not changed.

Signed-off-by: Ce Gao ce.gao@outlook.com


This change is Reviewable

@gaocegege gaocegege changed the title WIP: Remove TensorBoard related code in operator Remove TensorBoard related code in operator Feb 14, 2018
@jlewi
Copy link
Contributor

jlewi commented Feb 14, 2018

@gaocegege Do you want to assign a reviewer from Caicloud?

@gaocegege
Copy link
Member Author

gaocegege commented Feb 15, 2018

@jlewi

I think everyone could review it and most of us are on holiday now 😄

@jimexist
Copy link
Member

related to the release cycle of this repo - i think we need to give our audience some expectation of release cycles, and to delete some code they have to be deprecated for at least two or three releases so that people don't get broken code.

@jimexist
Copy link
Member

jimexist commented Feb 15, 2018

@gaocegege @jlewi i think you need to update the ref in https://coveralls.io/github/tensorflow/k8s, i.e. change the repo name.

@gaocegege
Copy link
Member Author

SGTM, then it could be blocked until that time

@jlewi
Copy link
Contributor

jlewi commented Feb 15, 2018

Tests should be fixed.
Please sync and rerun the tests.

@coveralls
Copy link

coveralls commented Feb 17, 2018

Coverage Status

Coverage decreased (-4.4%) to 27.475% when pulling 22b0c19 on gaocegege:tb into ab1cb4d on kubeflow:master.

@gaocegege
Copy link
Member Author

dashboard/backend/handler/api_handler.go:143: job.Spec.TensorBoard undefined (type "github.com/kubeflow/tf-operator/pkg/apis/tensorflow/v1alpha1".TFJobSpec has no field or method TensorBoard)

I will fix it.

@jlewi
Copy link
Contributor

jlewi commented Feb 21, 2018

/test all

@jlewi
Copy link
Contributor

jlewi commented Feb 26, 2018

@gaocegege Is this PR still blocked?

@gaocegege
Copy link
Member Author

No, I will update it soon.

Signed-off-by: Ce Gao <gaoce@caicloud.io>
Signed-off-by: Ce Gao <gaoce@caicloud.io>
Signed-off-by: Ce Gao <gaoce@caicloud.io>
@gaocegege gaocegege requested review from jlewi, wbuchwalter and jimexist and removed request for wbuchwalter February 27, 2018 03:32
@jlewi
Copy link
Contributor

jlewi commented Feb 27, 2018

Reviewed 14 of 18 files at r1, 5 of 5 files at r2.
Review status: 19 of 23 files reviewed at latest revision, 2 unresolved discussions.


examples/tf_job_tensorboard_azure.yaml, line 1 at r2 (raw file):

apiVersion: "kubeflow.org/v1alpha1"

We could probably just delete this file. I think the whole point of this file was to provide an example of TB on Azure. Now that we no longer support TB its not relevant.


pkg/apis/tensorflow/v1alpha1/types.go, line 55 at r2 (raw file):

	// TFImage defines the tensorflow docker image that should be used for default parameter server
	TFImage string `json:"tfImage,omitempty"`

Can we delete this now? Do we use this image for anything now? Maybe add a TODO.


Comments from Reviewable

@gaocegege
Copy link
Member Author

examples/tf_job_tensorboard_azure.yaml, line 1 at r2 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

We could probably just delete this file. I think the whole point of this file was to provide an example of TB on Azure. Now that we no longer support TB its not relevant.

SGTM


Comments from Reviewable

@gaocegege
Copy link
Member Author

pkg/apis/tensorflow/v1alpha1/types.go, line 55 at r2 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Can we delete this now? Do we use this image for anything now? Maybe add a TODO.

I think we could add a TODO and set the issue to be good first issue, and maybe we could attract more contributors. If no one is interested and I could fix it in another PR. WDYT

I think we should add more contributor-friendly issues in the repo. Now we only have some regular contributors from several companies, it is not good, IMO


Comments from Reviewable

Signed-off-by: Ce Gao <gaoce@caicloud.io>
@jlewi
Copy link
Contributor

jlewi commented Feb 27, 2018

:lgtm:


Reviewed 2 of 18 files at r1, 1 of 1 files at r3.
Review status: 22 of 23 files reviewed at latest revision, 1 unresolved discussion.


pkg/apis/tensorflow/v1alpha1/types.go, line 55 at r2 (raw file):

Previously, gaocegege (Ce Gao) wrote…

I think we could add a TODO and set the issue to be good first issue, and maybe we could attract more contributors. If no one is interested and I could fix it in another PR. WDYT

I think we should add more contributor-friendly issues in the repo. Now we only have some regular contributors from several companies, it is not good, IMO

A TODO seems good to me. I'm also fine with just filing an issue for this and not blocking this PR.


Comments from Reviewable

@jlewi
Copy link
Contributor

jlewi commented Feb 27, 2018

Reviewed 1 of 18 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@jlewi
Copy link
Contributor

jlewi commented Feb 27, 2018

/lgtm

@jlewi
Copy link
Contributor

jlewi commented Feb 27, 2018

/approve

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlewi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

1 similar comment
@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlewi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jlewi jlewi merged commit 0759f7a into kubeflow:master Feb 27, 2018
@gaocegege gaocegege deleted the tb branch February 28, 2018 01:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants