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

Update docs: add lib development guide #178

Merged
merged 1 commit into from
Nov 9, 2021

Conversation

JoeyHwong-gk
Copy link
Contributor

  • add lib development guide
  • Supplementary quickstart documentation
  • fix known issue

Signed-off-by: JoeyHwong joeyhwong@gknow.cn

@JoeyHwong-gk
Copy link
Contributor Author

/assign @llhuii @jaypume

@kubeedge-bot kubeedge-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 30, 2021
@JoeyHwong-gk
Copy link
Contributor Author

/cc @ugvddm @TymonXie @JimmyYang20

@kubeedge-bot kubeedge-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 30, 2021
SECRET_ACCESS_KEY: XXXXXXXX
stringData:
ACCESS_KEY_ID: Q3AM3UQ867SPQQA43P2F
SECRET_ACCESS_KEY: zuf+tfteSlswRu7BJ86wekitnifILbZam1KYY3TG
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove a specific account information, and replace it by {USER_AK}.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry about this mistake. I have fix the rebase error

@@ -96,6 +96,15 @@ type InitialModel struct {

type DeployModel struct {
Name string `json:"name"`
// HotUpdateEnabled will enable the model hot update feature if its value is true.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please create a new pr, don't mix into one pr with docs update

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry about this mistake. I have fix the rebase error

Copy link

@MooreZheng MooreZheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The writing of Quick Start still needs more efforts to make it more straightforward and reader-friendly.


### Prepare
- Install Sedna on a cluster Step By Step: [guide here](setup/install.md).
- Install Sedna AllinOne : [guide here](setup/local-up.md).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Need to briefly explain what is the difference between the "step by step" and "AllinOne".
  2. Do users really need to read "step by step" first before "AllinOne"? If so, it should be stated. If not, we shall make "AllinOne" self-contained: that is because readers may directly clicked into this link without reading and conducting "step by step".

Copy link
Contributor Author

@JoeyHwong-gk JoeyHwong-gk Aug 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do users really need to read "step by step" first before "AllinOne"
In the previous summary, the document explains that sedna provides two ways。


todo
### Component

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A "Quick-start" is suggested to be "quick". The introductions of components and system designs take too much space here which are in fact not necessary to run a increamental learning job.

  1. If we need to use some notations in the following paragraphs, e.g., local controller, it would be much smoother to introduce them until they are used, instead of putting all notations at the beginning, forcing them to remember all these. People usually forget, and they hate to read too much in a "Quick-start".
  2. If we must put some notations here, the writing could be much more straightforward by providing some notation links to the "Introduction" sections.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After understanding of the positioning of components in sedna can help users (not only developers) know their personal responsibilities during application development, and what and why it is necessary to be split during development

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same opinion, the introductions of framework really distracted non-k8s users, and it is impossible to understand by a beginner in this stage.


#### 1.1 Deployment Planning

In this example, there is only one host with two nodes, which had creating a Kubernetes cluster with `kind`.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kind is unfamiliar to non-k8s users. Besides, do we really need to mention Kubernetes cluster and kind here, given that Sedna is already deployed?

  1. If not
    Remove the sentence of "which had creating a Kubernetes cluster with kind" (and put a link to "Deploying Sedna" when needed).
  2. If so
    2.1. Notation explaination is needed.
    2.2. Shall it be "which is a Kubernetes cluster created with kind." instead of "which had creating a Kubernetes cluster with kind"?

Copy link
Contributor Author

@JoeyHwong-gk JoeyHwong-gk Aug 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the prepare chapter, kind is one of the ways to install sedna. This section describes the service environment in our demo which prepared by kind.

##### 2.3.1 Encapsulate an Estimators

Sedna implements several pre-made Estimators in [example](/examples), your can find them from the python scripts called `interface`.
Sedna supports the Estimators build from popular AI frameworks, such as TensorFlow, Pytorch, PaddlePaddle, MindSpore. Also Custom estimators can be used according to our interface document.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Custom estimator is important, but it can take quite a few effort in the "Quick Start" and not needed to detail it with so much space here: a link is already sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Custom estimator is important, but it can take quite a few effort in the "Quick Start" and not needed to detail it with so much space here: a link is already sufficient.

estimator is an abstract concept that's been introduced by sedna, without detail, the fear is that it will not be understood.


```

- `Context` provides the capability of obtaining the context from CRD

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CRD is unfamiliar to non-k8s users. A brief notation explaination or link is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I will replace it with an internal link

train_data.y.append(item["label"].asnumpy())
```

- `sedna.core` contain all edge-cloud features, Please note that each feature has its own parameters.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Typo. Please -> please
  2. "Each feature has its own parameters." It is confusing for a machine-learning developer, taking seconds to learn what is happening here. We do not mean the machine-learning feature of samples, right? Just say the incremental-learning job.

Copy link
Contributor Author

@JoeyHwong-gk JoeyHwong-gk Aug 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feature here means incremental-learning , lifelong-learning... In different feature, the initial parameters is need to pay attention. I'm eager for your opinion on "how to avoid the misunderstanding that users think this is a public parameter".


You can also find demos [here](/examples/incremental_learning/helmet_detection).

Some interfaces should be learn in job pipeline:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sentence has some thing wrong and hard to understand. We mean that there are APIs here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are the APIs that require the minimum necessary of developers

This image is generated by the script [build_images.sh](/examples/build_image.sh), used for creating training, eval and inference worker.

##### 3.2 Create Incremental Job
In this example, `$WORKER_NODE` is a custom node, you can fill it which you actually run.
Copy link

@MooreZheng MooreZheng Aug 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo:
In this example, $WORKER_NODE is a custom node, you can fill it which you actually run.
->
In this example, $WORKER_NODE is an environment variable indicating a customized node: you can fill its value with the one you actually run.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy from examples, shall we need to modify on both sides

### 4. Run

* incremental learning supports hot model updates and cold model updates. Job support
cold model updates default. If you want to use hot model updates, please to add the following fields:
Copy link

@MooreZheng MooreZheng Aug 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo:
Job support cold model updates default.
->
Job support cold model updates by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy from examples, shall we need to modify on both sides

```

This image is generated by the script [build_images.sh](/examples/build_image.sh), used for creating training, eval and inference worker.

### Create Incremental Job
in this example, `$WORKER_NODE` is a custom node, you can fill it which you actually run.
In this example, `$WORKER_NODE` is a custom node, you can fill it which you actually run.
Copy link

@MooreZheng MooreZheng Aug 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo:
In this example, $WORKER_NODE is a custom node, you can fill it which you actually run.
->
In this example, $WORKER_NODE is an environment variable indicating a customized node: you can fill its value with the one you actually run.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry about this mistake. I have fix the rebase error

@kubeedge-bot kubeedge-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 30, 2021
- add lib development guide
- Supplementary quickstart documentation
- fix known issue

Signed-off-by: JoeyHwong <joeyhwong@gknow.cn>

This image is generated by the script [build_images.sh](/examples/build_image.sh), used for creating training, eval and inference worker.

##### 3.2 Create Incremental Job
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you duplicate the example of incremental job here? How about just linking here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This document describes how to quickly develop an application based on sedna. After scripting is complete, the following describes how to run. Diff to example, we removed requirements and results.

- `datasources` base class, as that core feature of sedna require identifying the features and labels from data input, we specify that the first parameter for train/evaluate of the ML framework

```python
from sedna.datasources import BaseDataSource
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too many code here, and look like the quick start of lib.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can our community provide a description or discussion: what information should be included in quick start ?


### 1. Prepare

#### 1.1 Deployment Planning
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Deployment section is duplicate with https://github.com/kubeedge/sedna/blob/main/docs/setup/install.md
Better to let user just refer it.

@jaypume
Copy link
Member

jaypume commented Nov 5, 2021

/lgtm

@kubeedge-bot kubeedge-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 5, 2021
@JoeyHwong-gk
Copy link
Contributor Author

/hold

@kubeedge-bot kubeedge-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 5, 2021
@JoeyHwong-gk
Copy link
Contributor Author

/unhold

@kubeedge-bot kubeedge-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 9, 2021
@JoeyHwong-gk
Copy link
Contributor Author

/ping @jaypume

@jaypume
Copy link
Member

jaypume commented Nov 9, 2021

/lgtm

@jaypume
Copy link
Member

jaypume commented Nov 9, 2021

/approve

@kubeedge-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jaypume

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

@kubeedge-bot kubeedge-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 9, 2021
@kubeedge-bot kubeedge-bot merged commit 1d0f39b into kubeedge:main Nov 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants