tensorflow savedmodel warmup #539

datelier · 2020-07-02T04:39:36Z

Signed-off-by: datelier 57349093+datelier@users.noreply.github.com

Description:

tensorflow savedmodel warmup

Related Issue:

How Has This Been Tested?:

Environment:

Go Version: 1.14.3
Docker Version: 19.03.8
Kubernetes Version: 1.18.2
NGT Version: 1.11.5

Types of changes:

Bug fix [type/bug]
New feature [type/feature]
Add tests [type/test]
Security related changes [type/security]
Add documents [type/documentation]
Refactoring [type/refactoring]
Update dependencies [type/dependency]
Update benchmarks and performances [type/bench]
Update CI [type/ci]

Changes to Core Features:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your core changes, as applicable?
Have you successfully ran tests with your changes locally?

Checklist:

I have read the CONTRIBUTING document.
I have checked open Pull Requests for the similar feature or fixes?
I have added tests and benchmarks to cover my changes.
I have ensured all new and existing tests passed.
I have commented my code, particularly in hard-to-understand areas
I have updated the documentation accordingly.

pull-assistant · 2020-07-02T04:39:38Z

Best reviewed: commit by commit

Optimal code review plan (1 warning)

tensorflow savedmodel warmup

...erter/tensorflow/tensorflow.go 45% changes removed in fix warmup

fix warmup

fix DeepSource issue: Empty string test can be improved

fix test checkFunc

Merge branch 'master' into feature/internal/tensorflow_warmup

Powered by Pull Assistant. Last update fcb3cb7 ... ee4bb8b. Read the comment docs.

codecov · 2020-07-02T04:50:25Z

Codecov Report

Merging #539 into master will increase coverage by 3.95%.
The diff coverage is 17.10%.

@@            Coverage Diff             @@
##           master     #539      +/-   ##
==========================================
+ Coverage    7.84%   11.80%   +3.95%     
==========================================
  Files         388      407      +19     
  Lines       19945    21227    +1282     
==========================================
+ Hits         1565     2505     +940     
- Misses      18156    18445     +289     
- Partials      224      277      +53

Impacted Files	Coverage Δ
internal/cache/cacher/cacher.go	`100.00% <ø> (+100.00%)`	⬆️
internal/client/discoverer/discover.go	`0.00% <0.00%> (ø)`
internal/compress/gob.go	`26.19% <0.00%> (-1.31%)`	⬇️
internal/compress/gzip.go	`26.98% <0.00%> (-7.02%)`	⬇️
internal/compress/lz4.go	`36.36% <0.00%> (-5.31%)`	⬇️
internal/compress/zstd.go	`29.50% <0.00%> (-5.79%)`	⬇️
internal/config/blob.go	`0.00% <0.00%> (ø)`
internal/config/config.go	`15.78% <ø> (+15.78%)`	⬆️
internal/config/grpc.go	`0.00% <0.00%> (ø)`
internal/config/log.go	`100.00% <ø> (+100.00%)`	⬆️
... and 162 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc6a44e...ee4bb8b. Read the comment docs.

kpango · 2020-07-03T05:17:17Z

internal/core/converter/tensorflow/tensorflow.go

@@ -69,7 +70,17 @@ const (
 	threeDim
 )

-var loadFunc = tf.LoadSavedModel
+var loadFunc = func(t *tensorflow) error {


local func variable is not clean, please use receiver func instead of local func val

I think he want to override it during testing, so he defines this function as variable instead of the receiver function.

One more thing, this function is not tested ... (the coverage of this file is 86%)

exactly, that's why I recommend method & it can be replaced by Interface, please mock it using interface

vankichi · 2020-07-06T01:27:48Z

/rebase

vdaas-ci · 2020-07-06T01:31:12Z

[REBASE] Rebase triggered by vankichi for branch: feature/internal/tensorflow_warmup

hlts2 · 2020-07-07T01:12:40Z

internal/core/converter/tensorflow/option_test.go

+
+	for _, test := range tests {
+		t.Run(test.name, func(tt *testing.T) {
+			defer goleak.VerifyNone(t)


Suggested change

defer goleak.VerifyNone(t)

defer goleak.VerifyNone(tt)

kevindiu · 2020-07-07T01:07:29Z

internal/core/converter/tensorflow/option_test.go

+
+	for _, test := range tests {
+		t.Run(test.name, func(tt *testing.T) {
+			defer goleak.VerifyNone(t)


Suggested change

defer goleak.VerifyNone(t)

defer goleak.VerifyNone(tt)

datelier · 2020-07-09T05:37:23Z

/rebase

vdaas-ci · 2020-07-09T05:38:05Z

[REBASE] Rebase triggered by datelier for branch: feature/internal/tensorflow_warmup

Signed-off-by: datelier <57349093+datelier@users.noreply.github.com>

hlts2 · 2020-07-09T06:53:17Z

internal/core/converter/tensorflow/option_test.go

+		opts := []cmp.Option{
+			cmp.AllowUnexported(tensorflow{}),
+			cmp.AllowUnexported(OutputSpec{}),
+			cmpopts.IgnoreFields(tensorflow{}, "loadFunc"),
+		}
+		if diff := cmp.Diff(w.obj, obj, opts...); diff != "" {
+			return errors.Errorf("err: %s", diff)
+		}
+		opt := cmp.Comparer(func(want, obj T) bool {
+			p1 := reflect.ValueOf(want).FieldByName("loadFunc").Pointer()
+			p2 := reflect.ValueOf(obj).FieldByName("loadFunc").Pointer()
+			return p1 == p2
+		})
+		if !cmp.Equal(w.obj, obj, opt) {
+			return errors.Errorf("got = %v, want = %v", obj, w.obj)
+		}


Suggested change

opts := []cmp.Option{

cmp.AllowUnexported(tensorflow{}),

cmp.AllowUnexported(OutputSpec{}),

cmpopts.IgnoreFields(tensorflow{}, "loadFunc"),

}

if diff := cmp.Diff(w.obj, obj, opts...); diff != "" {

return errors.Errorf("err: %s", diff)

}

opt := cmp.Comparer(func(want, obj T) bool {

p1 := reflect.ValueOf(want).FieldByName("loadFunc").Pointer()

p2 := reflect.ValueOf(obj).FieldByName("loadFunc").Pointer()

return p1 == p2

})

if !cmp.Equal(w.obj, obj, opt) {

return errors.Errorf("got = %v, want = %v", obj, w.obj)

}

opts := []cmp.Option{

cmp.AllowUnexported(tensorflow{}),

cmp.AllowUnexported(OutputSpec{}),

cmpopts.IgnoreFields(tensorflow{}, "loadFunc"),

cmp.Comparer(func(want, got TF) bool {

p1 := reflect.ValueOf(want).Elem().FieldByName("loadFunc").Pointer()

p2 := reflect.ValueOf(got).Elem().FieldByName("loadFunc").Pointer()

return p1 == p2

}),

}

if diff := cmp.Diff(w.want, got, opts...); diff != "" {

return errors.Errorf("err: %s", diff)

}

Signed-off-by: datelier <57349093+datelier@users.noreply.github.com>

github-actions · 2020-07-09T07:07:28Z

internal/core/converter/tensorflow/tensorflow_test.go

@@ -42,74 +44,127 @@ func TestNew(t *testing.T) {
 		beforeFunc func(args)
 		afterFunc  func(args)
 	}
+	savedModel := &tf.SavedModel{}


[golangci] _{reported by reviewdog 🐶}
assignments should only be cuddled with other assignments (wsl)

github-actions · 2020-07-09T07:07:28Z

internal/core/converter/tensorflow/tensorflow_test.go

+		beforeFunc func()
+		afterFunc  func()
+	}
+	defaultCheckFunc := func(w want, err error) error {


[golangci] _{reported by reviewdog 🐶}
assignments should only be cuddled with other assignments (wsl)

github-actions · 2020-07-09T07:07:28Z

internal/core/converter/tensorflow/tensorflow_test.go

+				},
+				graph: tf.NewGraph(),
+				session: &mockSession{
+					RunFunc: func(feeds map[tf.Output]*tf.Tensor, fetches []tf.Output, operations []*tf.Operation) ([]*tf.Tensor, error) {


[golangci] _{reported by reviewdog 🐶}
line is 123 characters (lll)

hlts2

LGTM

kevindiu · 2020-07-09T08:46:35Z

/format

vdaas-ci · 2020-07-09T08:51:26Z

[FORMAT] Updating license headers and formatting go codes triggered by kevindiu.

kevindiu · 2020-07-13T06:18:25Z

internal/core/converter/tensorflow/option.go

+	return func(t *tensorflow) {
+		if warmupInputs != nil {
+			if t.warmupInputs != nil {
+				t.warmupInputs = append(t.warmupInputs, warmupInputs...)


thinking functional options should not append inputs instead of replacing the value only.
@hlts2 what do you think?

I understand your opinion.
but From a flexibility point of view, I would like to keep the current implementation.

kevindiu

LGTM

kpango

LGTM

github-actions bot added area/internal team/set SET team size/L type/feature New feature priority/medium labels Jul 2, 2020

datelier added team/core Core team and removed priority/medium labels Jul 2, 2020

datelier requested review from hlts2, kevindiu and kpango July 2, 2020 04:46

kpango reviewed Jul 3, 2020

View reviewed changes

vdaas-ci force-pushed the feature/internal/tensorflow_warmup branch from b400518 to 68beb90 Compare July 6, 2020 01:31

github-actions bot added the priority/medium label Jul 6, 2020

hlts2 reviewed Jul 7, 2020

View reviewed changes

kevindiu reviewed Jul 7, 2020

View reviewed changes

datelier force-pushed the feature/internal/tensorflow_warmup branch from 68beb90 to 45123c4 Compare July 9, 2020 05:13

datelier added 3 commits July 9, 2020 05:38

tensorflow savedmodel warmup

fcb3cb7

Signed-off-by: datelier <57349093+datelier@users.noreply.github.com>

fix warmup

722a9a9

Signed-off-by: datelier <57349093+datelier@users.noreply.github.com>

fix DeepSource issue: Empty string test can be improved

ba92ef5

Signed-off-by: datelier <57349093+datelier@users.noreply.github.com>

vdaas-ci force-pushed the feature/internal/tensorflow_warmup branch from 7b0f0d1 to ba92ef5 Compare July 9, 2020 05:38

hlts2 reviewed Jul 9, 2020

View reviewed changes

fix test checkFunc

34c4330

Signed-off-by: datelier <57349093+datelier@users.noreply.github.com>

github-actions bot reviewed Jul 9, 2020

View reviewed changes

hlts2 approved these changes Jul 9, 2020

View reviewed changes

kevindiu reviewed Jul 13, 2020

View reviewed changes

kevindiu approved these changes Jul 13, 2020

View reviewed changes

kpango added the status/author-review label Jul 14, 2020

rinx approved these changes Jul 16, 2020

View reviewed changes

kpango approved these changes Jul 22, 2020

View reviewed changes

Merge branch 'master' into feature/internal/tensorflow_warmup

ee4bb8b

kpango merged commit 6d215f6 into master Jul 23, 2020

kpango deleted the feature/internal/tensorflow_warmup branch July 23, 2020 05:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tensorflow savedmodel warmup #539

tensorflow savedmodel warmup #539

datelier commented Jul 2, 2020

pull-assistant bot commented Jul 2, 2020 •

edited

Loading

codecov bot commented Jul 2, 2020 •

edited

Loading

kpango Jul 3, 2020

kevindiu Jul 7, 2020

kpango Jul 7, 2020

vankichi commented Jul 6, 2020

vdaas-ci commented Jul 6, 2020

hlts2 Jul 7, 2020

kevindiu Jul 7, 2020

datelier commented Jul 9, 2020

vdaas-ci commented Jul 9, 2020

hlts2 Jul 9, 2020

github-actions bot Jul 9, 2020

github-actions bot Jul 9, 2020

github-actions bot Jul 9, 2020

hlts2 left a comment

kevindiu commented Jul 9, 2020

vdaas-ci commented Jul 9, 2020

kevindiu Jul 13, 2020

hlts2 Jul 13, 2020

kevindiu left a comment

kpango left a comment

tensorflow savedmodel warmup #539

tensorflow savedmodel warmup #539

Conversation

datelier commented Jul 2, 2020

Description:

Related Issue:

How Has This Been Tested?:

Environment:

Types of changes:

Changes to Core Features:

Checklist:

pull-assistant bot commented Jul 2, 2020 • edited Loading

codecov bot commented Jul 2, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vankichi commented Jul 6, 2020

vdaas-ci commented Jul 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datelier commented Jul 9, 2020

vdaas-ci commented Jul 9, 2020

Choose a reason for hiding this comment

github-actions bot Jul 9, 2020

Choose a reason for hiding this comment

github-actions bot Jul 9, 2020

Choose a reason for hiding this comment

github-actions bot Jul 9, 2020

Choose a reason for hiding this comment

hlts2 left a comment

Choose a reason for hiding this comment

kevindiu commented Jul 9, 2020

vdaas-ci commented Jul 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevindiu left a comment

Choose a reason for hiding this comment

kpango left a comment

Choose a reason for hiding this comment

pull-assistant bot commented Jul 2, 2020 •

edited

Loading

codecov bot commented Jul 2, 2020 •

edited

Loading