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

[RFC] Directory Refactor #495

Closed
gaocegege opened this issue May 9, 2019 · 12 comments
Closed

[RFC] Directory Refactor #495

gaocegege opened this issue May 9, 2019 · 12 comments

Comments

@gaocegege
Copy link
Member

gaocegege commented May 9, 2019

Now the directory layout is a little confusing:

./pkg
├── api
│   ├── health
│   ├── operators
│   │   └── apis
│   │       ├── common
│   │       │   └── v1alpha2
│   │       ├── experiment
│   │           └── v1alpha2
│   │       ├── studyjob
│   │       │   └── v1alpha1
│   │       └── trial
│   │           └── v1alpha2
│   ├── v1alpha1
│   └── v1alpha2
├── controller
│   ├── v1alpha1
│   │   └── studyjob
│   └── v1alpha2
│       ├── experiment
│       └── trial
├── db
│   ├── v1alpha1
│   └── v1alpha2
├── earlystopping
│   └── v1alpha1
├── manager
│   ├── modelstore
│   └── v1alpha1
│       ├── file-metricscollector
│       ├── metricscollector
│       ├── studyjobclient
│       └── visualise
│           └── tensorboard
├── mock
│   ├── modelstore
│   ├── v1alpha1
│   └── v1alpha2
├── suggestion
│   ├── v1alpha1
├── ui
└── util
    └── v1alpha2

I propose some changes:

  • Move pkg/mock/modelstore to pkg/mock/v1alpha1/db. Maybe we need to change the generate script to do it automatically.
  • Move pkg/manager/modelstore to pkg/manager/v1alpha1, since it is version specific implementation. And it will be changed in v1alpha2.
  • Change pkg/controller/v1alpha2/experiment to pkg/controller/experiment/v1alpha2/. Becuase the API layout is pkg/api/operators/apis/experiment/v1alpha2
  • Change pkg/controller/v1alpha2/trial to pkg/controller/trial/v1alpha2/
  • Change pkg/controller/v1alpha1/studyjob to pkg/controller/studyjob/v1alpha2/
  • Rename pkg/api/operators to pkg/apis/controller/. The new name follows the same convention with pkg/controller
  • Put pkg/api/v1alpha1 and pkg/api/v1alpha2 in pkg/apis/manager. At the same time, make API proto about suggestions in pkg/apis/suggestion/v1alpha1 and pkg/apis/suggestion/v1alpha2. Becuase it will be called directly in v1alpha2.

The new layout will look like:

./pkg
+── apis
-── api
│   ├── health
-   ├── operators
+   ├── controller
│   │   ├── common
│   │   │   └── v1alpha2
-   │   ├── experiment
+   │   ├── experiments
│   │   │   └── v1alpha2
│   │   ├── studyjob
│   │   │   └── v1alpha1
-   │   └── trial
+   │   └── trials
│   │       └── v1alpha2
│   ├── v1alpha1
│   └── v1alpha2
├── controller
-   ├── v1alpha1
-   │   └── studyjob
-   └── v1alpha2
-       ├── experiment
-       └── trial
+   ├── studyjob
+   │   └── v1alpha1
+   └── experiment
+       ├── v1alpha2
+   └── trial
+       └── v1alpha2
...
├── manager
-   ├── modelstore
│   └── v1alpha1
│       ├── file-metricscollector
│       ├── metricscollector
│       ├── studyjobclient
│       └── visualise
│           └── tensorboard
├── mock
-   ├── modelstore
│   ├── v1alpha1
│   └── v1alpha2
...
@gaocegege
Copy link
Member Author

gaocegege commented May 9, 2019

@YujiOshima
Copy link
Contributor

Though currently, health is only used in the manager, it can be used in all services.
It is based on the offical grpc health check service https://github.com/grpc/grpc/blob/master/doc/health-checking.md

@gaocegege
Copy link
Member Author

@YujiOshima Make sense. Then I think we should keep it. I updated the proposal, Thanks for your reminder

@hougangliu
Copy link
Member

it can make katib directory style consistent now.
modelstore is not in use now, should we drop it? or make a backup directory in root path to store it momentarily since I am not sure if modeldb storage plugin can reuse part of the code

@gaocegege
Copy link
Member Author

Is modelstore used in v1alpha1?

@johnugeorge
Copy link
Member

Model store is currently not used. Most of the code is obsolete and it is written for v1alpha1 api. I don't think, we need to keep the code. @YujiOshima

@gaocegege
Copy link
Member Author

Then maybe we could just delete it. model store interface confuses me for a long time.

@richardsliu
Copy link
Contributor

Do we need to do this for v1alpha2? This can cause a lot of conflicts with ongoing PRs.

@gaocegege
Copy link
Member Author

It is just a suggestion. We can do it after v1alpha2 is released.

@YujiOshima
Copy link
Contributor

@johnugeorge Yes, the Model store was used before v0.4. We can drop it.
@gaocegege Agree, we should do this after making v1alpha2 stable.

@gaocegege
Copy link
Member Author

#502 (comment)

I feel, Kubebuilder recommends plural form of CR. Ideally, it should have been apis/experiments, apis/trials as per Kubebuilder notation. I was planning to take this up post 0.6 release

We should do it, too.

@gaocegege
Copy link
Member Author

/close by #737

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

No branches or pull requests

5 participants