Skip to content
This repository has been archived by the owner on Sep 12, 2023. It is now read-only.

Enhance maintainability of operator common module #54

Closed
Jeffwan opened this issue Mar 9, 2020 · 5 comments
Closed

Enhance maintainability of operator common module #54

Jeffwan opened this issue Mar 9, 2020 · 5 comments
Assignees

Comments

@Jeffwan
Copy link
Member

Jeffwan commented Mar 9, 2020

There're few issues in current project and I propose to have some improvements for these issues.
I am in progress of a PR and please share some insights and feedbacks.

  1. ./hack/update-codegen doesn't work with current code structure. It tries to generate outputs for operator:v1 but project only have job_controller/api/v1 directory which doesn't match.
    github.com/kubeflow/common/client github.com/kubeflow/common \
    operator:v1 \

Either the update-codegen or code structure need to be changed.

  1. There's no ./hack/verify-codegen.sh codegen verify process in the CI.

  2. code-generator version referenced in the project doesn't work. The minimum version to support go mod is 0.15.x
    https://github.com/kubeflow/common/blob/88a0cd071cf162120aa3ea0c3cb9cc64d69ef3f5/hack/update-codegen.sh#L25

  3. Upgrade Kubernetes dependency.
    common project uses expectation which belongs to k8s.io/kubernetes which makes it has direct dependency on it.

    Expectations controller.ControllerExpectationsInterface

    We should start to consider to remove dependency of k8s/kubernetes because it is not primarily intended to be consumed as a module. Only the published subcomponents are. reference: Delete direct dependency of kubernetes #48

The other challenge is most of the new training operators may use kubebuilder2 or latest operator-sdk to generate APIs. These framework depends on latest k8s.io/code-generater and k8s.io/client-go which has conflicts with the ones k8s.io/kubernetes@1.11 uses. There're some breaking changes user have to resolve.

It would be better to upgrade to 1.15+ for all k8s.io/xx dependencies and make sure our common package is compatible with newer client-go.

  1. code struct is not that clear. Looks like apis is under job_controller, I think we plan to have ControllerInterface and common codes under this package, As I know, some users only use api like CleanupPolicy or RestartPolicy, etc. It would be great to separate api definition and jobcontroller implementation in different folders.

For test_job, seems this is only used for testing and I think it's fine to put apis and client under pkg directly. I am ok to have a separate top level directory like test_job to host apis. Currently we have this codes structure for test_job.

├── client
│   ├── clientset
│   │   └── versioned
│   ├── informers
│   │   └── externalversions
│   └── listers
│          └── test_job
├── test_job
│   ├── v1

Based on what we talked above, could we create a project architecture like this


├── LICENSE
├── OWNERS
├── README.md
├── go.mod
├── go.sum
├── hack
│   ├── boilerplate
│   ├── scripts
│   ├── update-codegen.sh
│   └── verify-codegen.sh
├── pkg
│   ├── apis
│   │   └── common
│   │       └── v1              ---> common API 
│   │   └── test_job
│   │       └── v1             ----> test Job API
│   ├── client                   ----> test Job client
│   │   ├── clientset
│   │   │   └── versioned
│   │   ├── informers
│   │   │   └── externalversions
│   │   └── listers
│   │       └── test_job
│   ├── job_controller
│   │   ├── job.go
│   │   ├── job_controller.go
│   │   ├── job_test.go
│   │   ├── pod.go
│   │   ├── pod_control.go
│   │   ├── pod_control_test.go
│   │   ├── pod_test.go
│   │   ├── service.go
│   │   ├── service_control.go
│   │   ├── service_control_test.go
│   │   ├── service_ref_manager.go
│   │   ├── service_ref_manager_test.go
│   │   ├── status.go
│   │   ├── status_test.go
│   │   ├── test_job_controller.go
│   │   ├── util.go
│   │   └── util_test.go
│   ├── test_util                          ----> I doubt some of utils still being used, need more clean ups.
│   │   ├── const.go..
│   └── util
│       ├── k8sutil
│       ├── logger.go
│       ├── signals
│       ├── status.go
│       ├── status_test.go
│       ├── train
│       └── util.go


For other operators, they can import package api and controller packages like following.

import (
"github.com/kubeflow/common/pkg/apis/common/v1"
"github.com/kubeflow/common/pkg/job_controller"
)

Please leave some comments, I will file the PR if it makes sense.

/cc @terrytangyuan @gaocegege @jian-he @richardsliu

@issue-label-bot
Copy link

Issue-Label Bot is automatically applying the labels:

Label Probability
feature 0.87

Please mark this comment with 👍 or 👎 to give our bot feedback!
Links: app homepage, dashboard and code for this bot.

1 similar comment
@kf-label-bot-dev
Copy link

Issue-Label Bot is automatically applying the labels:

Label Probability
feature 0.87

Please mark this comment with 👍 or 👎 to give our bot feedback!
Links: app homepage, dashboard and code for this bot.

@Jeffwan
Copy link
Member Author

Jeffwan commented Mar 9, 2020

@terrytangyuan
Copy link
Member

@Jeffwan Thanks for sharing the detailed plan! The list of suggested enhancements makes sense to me. We can review the details in PRs.

@Jeffwan
Copy link
Member Author

Jeffwan commented Apr 15, 2020

/assign @Jeffwan

All issues haven been resolved besides removing kubernetes dependency of expectations. This can be done in separate PR and will use track this issue in #48

@Jeffwan Jeffwan closed this as completed Apr 15, 2020
georgkaleido pushed a commit to georgkaleido/common that referenced this issue Jun 9, 2022
* Add test_backgrounds

* Add test_channels, test_color_consistency, test_color_spaces

* Add test_dpi, test_fail, Fix tests

* Add test_crop

* Add test_format

* Add test_orientation

* Add test_position

* Add test_roi

* Add test_scale

* Add test_semitransparent

* Add test_serialops

* Add test_shadow

* Add test_transparent_color

* Add test_type

* Update requirements to include core server tester

* Update conftest.py

* Add test_size

* Add twine circleci dependency

* Update requirements

* Update readme on how to run tests

* Update readme, Use core test utils

* Update requirements after rebase

* Fix readme

* Fix yaml danger stuff

Co-authored-by: Paul Angerer <dabauxi@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants