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

[RELAY]Frontend darknet #2773

Merged
merged 4 commits into from
May 24, 2019
Merged

[RELAY]Frontend darknet #2773

merged 4 commits into from
May 24, 2019

Conversation

siju-samuel
Copy link
Member

@siju-samuel siju-samuel commented Mar 11, 2019

#2246
@srkreddy1238 @zhreshold @jroesch @yzhliu @kazum Please help to review the darknet frontend.

Thanks for contributing to TVM! Please refer to guideline https://docs.tvm.ai/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers.

@jroesch
Copy link
Member

jroesch commented Mar 11, 2019

Overall looks good to me, will try to take a deeper dive later.

test_forward_upsample()
test_forward_l2normalize()
test_forward_activation_logistic()
test_forward_elu()
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update tests/scripts/task_python_frontend.sh too so that CI can run this test file.

@jroesch
Copy link
Member

jroesch commented Mar 17, 2019

@kazum can you give explicit approval if it looks good to you?

@jroesch jroesch requested a review from kazum March 17, 2019 02:55
urllib.urlretrieve(url, path)

DARKNET_LIB = 'libdarknet2.0.so'
DARKNETLIB_URL = 'https://github.com/siju-samuel/darknet/blob/master/lib/' \
Copy link
Member

Choose a reason for hiding this comment

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

Can we put this in a centrally located place? there is a TVM data repo somewhere, can we download from Joe's repo?


else:
err = "Darknet layer type {} is not supported in relay.".format(layer_type)
raise NotImplementedError(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

DECONVOLUTIONAL and BATCHNORM are not handled. Tests for those layers are also necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

None of the available darknet cfg uses deconvolutional or batchnorm as independant op. And darknet lib donot support resize_layer for this 2 ops, so we cannot validate the testcases also for this. Currently the code is removed and can be added if any new model support this 2 ops.

processed = False

layer_type = LAYERTYPE(layer.type)
if LAYERTYPE.RNN == layer_type:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is RNN supported in this PR? If yes, please add tests for that.

from tvm.contrib import graph_runtime
from nnvm import frontend
from nnvm.testing.darknet import LAYERTYPE
from nnvm.testing.darknet import __darknetffi__
Copy link
Contributor

Choose a reason for hiding this comment

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

I think nnvm.testing.darknet should be moved to tvm.relay.testing.darknet like the tensorflow frontend.

from nnvm import frontend
from nnvm.testing.darknet import LAYERTYPE
from nnvm.testing.darknet import __darknetffi__
import nnvm.compiler
Copy link
Contributor

Choose a reason for hiding this comment

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

nnvm.frontend and nnvm.compiler are not necessary.

'''test elu activation layer'''
net = LIB.make_network(1)
layer_1 = LIB.make_convolutional_layer(1, 224, 224, 3, 32, 1, 3, 2, 0, 1, 0, 0, 0, 0)
layer_1.activation = 8
Copy link
Contributor

Choose a reason for hiding this comment

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

8 should be ACTIVATION.ELU here. Can you replace all the magic numbers for activation types with ACTIVATION.*?

import sys
import numpy as np
import matplotlib.pyplot as plt
from ctypes import *
Copy link
Contributor

Choose a reason for hiding this comment

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

sys and ctypes look not necessary.

@yzhliu
Copy link
Member

yzhliu commented Mar 29, 2019

@siju-samuel would you follow up the comments?

split_res2 = split_res[2]
if softmax:
split_res3 = get_relay_op('softmax')(split_res[3], axis=2)
out = get_relay_op('concatenate')((split_res0, split_res[1], split_res2, split_res3), axis=2)
Copy link
Contributor

Choose a reason for hiding this comment

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

split_res3 is not defined when softmax is False.

@kazum kazum added status: need update need update based on feedbacks and removed status: need review labels Mar 30, 2019
@siju-samuel siju-samuel force-pushed the relay_darknet branch 7 times, most recently from 1987bec to fd0c6a3 Compare April 19, 2019 03:19
@siju-samuel
Copy link
Member Author

@kazum Thanks for the review, I have updated the comments. Could u please help to review this again. TIA

extra_pad_size = attrs.get('extra_pad_size', 0)
if extra_pad_size:
pad_width = ((0, 0), (0, 0), (0, extra_pad_size), (0, extra_pad_size))
inputs = get_relay_op('pad')(*inputs,
Copy link
Contributor

@cbalint13 cbalint13 Apr 25, 2019

Choose a reason for hiding this comment

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

Will throw an error on yolov3-tiny (issue #3057)

tvm/relay/frontend/darknet.py", line 66, in _darknet_maxpooling
    return get_relay_op('max_pool2d')(*inputs, **new_attrs)
TypeError: max_pool2d() argument after * must be an iterable, not Call

A possible fix that works:

@@ -60,9 +60,9 @@
     extra_pad_size = attrs.get('extra_pad_size', 0)
     if extra_pad_size:
         pad_width = ((0, 0), (0, 0), (0, extra_pad_size), (0, extra_pad_size))
-        inputs = get_relay_op('pad')(*inputs,
+        inputs = [get_relay_op('pad')(*inputs,
                                      pad_width=pad_width,
-                                     pad_value=np.finfo(np.float32).min)
+                                     pad_value=np.finfo(np.float32).min)]
     return get_relay_op('max_pool2d')(*inputs, **new_attrs)
 
 def _darknet_avgpooling(inputs, params, attrs, prefix):

With this, as bonus, we just fixed issue #3057 partially using relay func instead of nnvm syms (yolov3-tiny, cuda version) !

python/tvm/relay/frontend/darknet.py Show resolved Hide resolved
# under the License.
"""
Compile YOLO-V2 and YOLO-V3 in DarkNet Models
=================================
Copy link
Contributor

Choose a reason for hiding this comment

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

Doc formatting.

# under the License.
"""
Test Darknet Models
=====================
Copy link
Contributor

Choose a reason for hiding this comment

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

Doc formatting.

# Download cfg and weights file if first time.
CFG_NAME = MODEL_NAME + '.cfg'
WEIGHTS_NAME = MODEL_NAME + '.weights'
REPO_URL = 'https://github.com/siju-samuel/darknet/blob/master/'
Copy link
Contributor

Choose a reason for hiding this comment

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

Use common location here too.

[neth, netw] = shape['data'][2:] # Current image shape is 608x608
######################################################################
# Load a test image
# --------------------------------------------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

doc formatting

# --------------------------------------------------------------------
test_image = 'dog.jpg'
print("Loading the test image...")
img_url = 'https://github.com/siju-samuel/darknet/blob/master/data/' + \
Copy link
Contributor

Choose a reason for hiding this comment

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

Use from common location

tvm.relay.testing.yolo_detection.do_nms_sort(dets, last_layer.classes, nms_thresh)

coco_name = 'coco.names'
coco_url = 'https://github.com/siju-samuel/darknet/blob/master/data/' + coco_name + '?raw=true'
Copy link
Contributor

Choose a reason for hiding this comment

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

common location here too.

@tqchen
Copy link
Member

tqchen commented May 16, 2019

This PR has been relatively inactive. @siju-samuel please act based on review comments.

@cbalint13
Copy link
Contributor

@siju-samuel,

Can get this in ? After this PR i would add yolo-tiny variant with tuning to your example. I am getting 123ms on mali with float16, probably same on plain arm cpu too. Perhaps can get an android demo too after it.

@siju-samuel
Copy link
Member Author

@cbalint13 i will update the PR by tomorrow. Tied up with some other work.

@kazum
Copy link
Contributor

kazum commented May 23, 2019

Sorry for being away for long time. The final version looks good to me.
@srkreddy1238 @jroesch @cbalint13, can you give approval if it looks good to you?

@kazum kazum added status: need review and removed status: need update need update based on feedbacks labels May 23, 2019
@cbalint13
Copy link
Contributor

Sorry for being away for long time. The final version looks good to me.
@srkreddy1238 @jroesch @cbalint13, can you give approval if it looks good to you?

  • I underlined a sigle functional bug in earlier stage of PR, was already fixed, have no more mentions. Glad to see the PR merged.

Copy link
Contributor

@srkreddy1238 srkreddy1238 left a comment

Choose a reason for hiding this comment

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

Thanks @siju-samuel. LGTM.

@tqchen
Copy link
Member

tqchen commented May 24, 2019

will leave the PR management to @kazum

@kazum kazum merged commit d3958e1 into apache:master May 24, 2019
@kazum
Copy link
Contributor

kazum commented May 24, 2019

Thanks @siju-samuel @srkreddy1238 @cbalint13 @jroesch, this is now merged!

wweic pushed a commit to wweic/tvm that referenced this pull request Jun 26, 2019
* [RELAY]Frontend darknet

* CI test file updated & CI error fixed

* avg_pool pad fix

* Changed repo_url and doc formatting
wweic pushed a commit to neo-ai/tvm that referenced this pull request Jun 27, 2019
* [RELAY]Frontend darknet

* CI test file updated & CI error fixed

* avg_pool pad fix

* Changed repo_url and doc formatting
@apivovarov
Copy link
Contributor

@siju-samuel Yolov3 works fine, but yolov2 fails with OverflowError.
Related discussion: https://discuss.tvm.apache.org/t/yolov2-tutorial-overflowerror/10495

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

Successfully merging this pull request may close these issues.

8 participants