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

Build a high-frequency Sedna-based end-to-end use case in ModelBox fo… #368

Merged
merged 1 commit into from
Oct 31, 2022

Conversation

Ymh13383894400
Copy link
Contributor

…rmat

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

@kubeedge-bot kubeedge-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 28, 2022
@kubeedge-bot kubeedge-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 17, 2022
@kubeedge-bot kubeedge-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 17, 2022
@@ -0,0 +1,28 @@
FROM modelbox/modelbox-develop-tensorflow_2.6.0-cuda_11.2-ubuntu-x86_64:latest
Copy link
Member

Choose a reason for hiding this comment

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

A README file should be added here, to introduce how to use this example.

WORKDIR /home/work
COPY ./lib /home/lib

COPY examples/incremental_learning/helmet_detection/training/ /home/work/
Copy link
Member

Choose a reason for hiding this comment

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

It should be declare in README that How to configure this part code to make it work with ModelBobx.

Copy link

@seveirbian seveirbian left a comment

Choose a reason for hiding this comment

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

Please check these comments and fix them.

@@ -0,0 +1,28 @@
FROM modelbox/modelbox-develop-tensorflow_2.6.0-cuda_11.2-ubuntu-x86_64:latest

Choose a reason for hiding this comment

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

  1. What is the difference between this Dockerfile and another Dockerfile in examples directory? Are they duplicated?
  2. Another question not about this Dockerfile, is incremental_learning-modelbox directory and files under it are duplicated compared to files in incremental_learning/helmet-detection-modelbox directory?

import warnings

import cv2
import numpy as np

Choose a reason for hiding this comment

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

Make imports more clean and in order.

#include "gmock/gmock.h"
#include "gtest/gtest.h"
#include "modelbox/data_context.h"
#include "modelbox/session_context.h"

Choose a reason for hiding this comment

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

Make includes clean and in order.

#include <modelbox/flow.h>
#include <iostream>
#include <string>
#include "test_config.h"

Choose a reason for hiding this comment

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

Make includes clean and in order.


#include "gtest/gtest.h"
#include "modelbox/base/log.h"
#include "modelbox/base/utils.h"

Choose a reason for hiding this comment

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

Make includes clean and in order.

@@ -0,0 +1,35 @@
#
Copy link
Member

Choose a reason for hiding this comment

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

src/flowunit/helm_infer/saved_model.pb is large file, and it should not be committed to this repository.
It is suggestted to put the file to GoogleDrive or elsewhere, and put the link here.

@@ -0,0 +1,35 @@
#
Copy link
Member

Choose a reason for hiding this comment

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

Why there are so many CMakeLists.txt files? Is it needed?
If it is, please give some explaination.


return modelbox.Status.StatusCode.STATUS_SUCCESS

def close(self):
Copy link
Member

Choose a reason for hiding this comment

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

Please add comment for those functions like this, otherwise it is not readable.
For example, why this function directly returns the status while the function name "close" looks like an action, not a query.

import numpy as np

# from sedna.common.config import Context
# from sedna.common.file_ops import FileOps
Copy link
Member

@jaypume jaypume Oct 25, 2022

Choose a reason for hiding this comment

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

Unused code should not be commited to repository. Please delete this part of code if it is really not used.





Copy link
Member

Choose a reason for hiding this comment

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

So many empty lines is not suggestted, please follow the standard code style, for example google python code style, or you can use the IDE to format the code.


foreach(subdir ${SUBDIRS})
add_subdirectory(${subdir})
endforeach()
Copy link
Member

Choose a reason for hiding this comment

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

Please keep a new line at the end of the file.

@@ -0,0 +1,86 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

It is suggestted to add user guide to introduce what is the function of each file like this.

@@ -0,0 +1,60 @@
#
Copy link
Member

Choose a reason for hiding this comment

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

It is better not to directly copy thirdparty source file here, please try to make these as dependency.
You can consider to move them to Dockerfile.

name: train-worker
imagePullPolicy: IfNotPresent
args: ["train.py"]
env:
Copy link
Contributor

Choose a reason for hiding this comment

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

Clear these variables if not used

2. Connect to the ModelBox orchestration service

```shell
modelbox-tool develop -q
Copy link
Contributor

Choose a reason for hiding this comment

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

modelbox-tool develop is unnecessary in demo test

### Prepare Model

```
链接:https://pan.baidu.com/s/1Gi5BJ_NQzqj66R8N5OXPzA
Copy link
Contributor

Choose a reason for hiding this comment

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

replace with English

metadata:
name: incremental-dataset
spec:
url: "/data/train_data.txt"
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't find this file in the link you provided

name: infer-worker
imagePullPolicy: IfNotPresent
args: ["test_mnist.py"]
env:
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems unnecessary

format = "graphviz"
graphconf = '''digraph mnist_sample {
node [shape=Mrecord]
httpserver_sync_receive[type=flowunit, flowunit=httpserver_sync_receive, device=cpu, time_out_ms=5000, endpoint="http://0.0.0.0:8190", max_requests=100]
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to redirect the port to receive http requests

node [shape=Mrecord]
httpserver_sync_receive[type=flowunit, flowunit=httpserver_sync_receive, device=cpu, time_out_ms=5000, endpoint="http://0.0.0.0:8190", max_requests=100]
mnist_preprocess[type=flowunit, flowunit=mnist_preprocess, device=cpu]
mnist_infer[type=flowunit, flowunit=mnist_infer, device=cpu, deviceid=0, batch_size=1]
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems sedna.lib not imported

@kubeedge-bot
Copy link
Collaborator

@JoeyHwong-gk: changing LGTM is restricted to collaborators

In response to this:

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.

@kubeedge-bot kubeedge-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 31, 2022
@JoeyHwong-gk
Copy link
Contributor

/lgtm

@kubeedge-bot
Copy link
Collaborator

@JoeyHwong-gk: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

Signed-off-by: Ymh13383894400 <1431605505@qq.com>
@jaypume
Copy link
Member

jaypume commented Oct 31, 2022

/lgtm

@kubeedge-bot kubeedge-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 31, 2022
@jaypume
Copy link
Member

jaypume commented Oct 31, 2022

/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 Oct 31, 2022
@kubeedge-bot kubeedge-bot merged commit d1cf42c into kubeedge:main Oct 31, 2022
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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants