Remove tf-operator from the codebase #1378
Conversation
With training-operator.v1 in place, we no longer need tf-operator.v1
The tests were effectively testing the contract of the testutil package so it makes sense the tests exist within the package itself.
With TFReconciler in place ce can remove the TFController and associated tests.
With tf_operator code deleted, we no longer need the dockerfile to build an image to run tf_operator.
Substitute the binary from tf-operator to training-operator.
- Substitute references to tf-operator with training-operator - Add instructions to apply all job CRDs instead of just TFJob as the operator expects them all to be present.
|
Hi @thunderboltsid. Thanks for your PR. I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
Thank you for taking this @thunderboltsid!
I left few comments
| @@ -169,7 +169,7 @@ def build_operator_image(root_dir, | |||
| # List of paths to copy relative to root. | |||
| sources = [ | |||
| "build/images/tf_operator/Dockerfile", "examples/tf_sample/tf_smoke.py", | |||
| os.path.join(go_path, bin_path, "tf-operator.v1"), | |||
| os.path.join(go_path, bin_path, "training-operator.v1"), | |||
There was a problem hiding this comment.
Do we need release.py file ? We probably need to check which files we are using from py/kubeflow/tf_operator.
cc @kubeflow/wg-training-leads
There was a problem hiding this comment.
The only release.py I can see referenced from the docs is https://github.com/kubeflow/tf-operator/blob/master/docs/release/release.py
There was a problem hiding this comment.
This file doesn't use py/kubeflow/..., right ?
There was a problem hiding this comment.
it does not.
There was a problem hiding this comment.
We are currently not using this script anymore. I think it should be ok to remove tf-operator reference first. Another story is needed to clean up test/release tools
|
/ok-to-test |
replace kubectl apply commands with makefile targets.
Metric counters have been refactored into pkg/common/metrics.go. This diff removes the stale counters present in tfjobcontroller.
|
/cc @Jeffwan @gaocegege |
|
/retest |
|
/hold I will take some time to review. |
As per review comments, we don't need to move the util_tests.go in pkg/common/util/testutil/util_test.go
Add remark about the origin file for the helper methods that were moved during the refactor in pkg/tensorflow.
|
Thanks for addressing the feedbacks. Overall looks good to me. Other reviewers? @andreyvelich @johnugeorge /lgtm |
delete variables no longer in use.
tfjob-operator is more consistent with the naming of other operators such as pytorchjob-operator or xgboostjob-operator
|
/test kubeflow-tf-operator-presubmit |
|
Thank you for this update! |
|
/retest |
|
/test kubeflow-tf-operator-presubmit |
- xgboostjob-operator -> xgboostjob-controller - mxnet-operator -> mxjob-controller - pytorchjob-operator -> pytorchjob-controller - tfjob-operator -> tfjob-controller
thunderboltsid
force-pushed
the
remove-tf-operator-cmd
branch
from
110cba7 to
7fdfa19
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jasonliu747, Jeffwan 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 |
|
/hold cancel |
Remove tf-operator and update the docs as per
#1367