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

Clean up the whole of training-operator #1816

Open
4 of 5 tasks
tenzen-y opened this issue May 31, 2023 · 21 comments · Fixed by #1847
Open
4 of 5 tasks

Clean up the whole of training-operator #1816

tenzen-y opened this issue May 31, 2023 · 21 comments · Fixed by #1847

Comments

@tenzen-y
Copy link
Member

tenzen-y commented May 31, 2023

After merging kubeflow/common into this repo, we have many redundant and duplicated codes in this repo.
So, we should clean up the whole of this repo.

/kind cleanup

ref: #1813

cc: @johnugeorge @andreyvelich

@tenzen-y
Copy link
Member Author

I take Remove TestJobController.

@tenzen-y
Copy link
Member Author

tenzen-y commented Jul 4, 2023

/reopen

@google-oss-prow google-oss-prow bot reopened this Jul 4, 2023
@google-oss-prow
Copy link

@tenzen-y: Reopened this issue.

In response to this:

/reopen

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.

@johnugeorge
Copy link
Member

As utils are cleaned up, Is there something pending in this issue?

@tenzen-y
Copy link
Member Author

tenzen-y commented Jul 31, 2023

As utils are cleaned up, Is there something pending in this issue?

@johnugeorge I think we need to decide how we handle the reconciler because the reconciler is dead codes.

@zw0610
Copy link
Member

zw0610 commented Aug 3, 2023

As utils are cleaned up, Is there something pending in this issue?

@johnugeorge I think we need to decide how we handle the reconciler because the reconciler is dead codes.

I think we can just remove the reconciler part.

@kuizhiqing
Copy link
Member

@zw0610 This part aim to give a unified controller implementation which do not depend on specific kind of job, am I right ?
If so, the next version of real UNIFIED training operator would profit it.

@tenzen-y
Copy link
Member Author

tenzen-y commented Aug 3, 2023

which do not depend on specific kind of job

@kuizhiqing Currently, this is the responsibility of the controller. The reconciler provides functionality the similar to the controller using the concept inspired by controller-runtime.

Although the reconciler's idea is much great, the reconciler is no longer maintained, a dead code.

So I would suggest removing the reconciler.
In the future, maybe we can implement common codes in a more controller-runtime native form.

@tenzen-y
Copy link
Member Author

tenzen-y commented Aug 3, 2023

I added the last task, Completely remove test_job, to this issue description.

@tenzen-y
Copy link
Member Author

tenzen-y commented Aug 3, 2023

/assign @tenzen-y @johnugeorge

@kuizhiqing
Copy link
Member

@tenzen-y I think are aligned in this topic. I developed operators with controller-runtime heavily, so I think we do not fully take the advantage of it here. And I am agree with you that,

  • we should remove reconciler here
  • in the future we may introduce it with more native way

@tenzen-y
Copy link
Member Author

@tenzen-y I think are aligned in this topic. I developed operators with controller-runtime heavily, so I think we do not fully take the advantage of it here. And I am agree with you that,

  • we should remove reconciler here
  • in the future we may introduce it with more native way

Yes, that's right. Thanks for being aligned.

Copy link

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Copy link

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@tenzen-y
Copy link
Member Author

/remove-lifecycle remove

I'm still working on the last piece.

@tenzen-y
Copy link
Member Author

/remove-lifecycle stale

Copy link

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@andreyvelich
Copy link
Member

/good-first-issue
/help

Copy link

@andreyvelich:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/good-first-issue
/help

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-sigs/prow repository.

Copy link

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@tenzen-y
Copy link
Member Author

tenzen-y commented Sep 3, 2024

/remove-lifecycle stale

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants