-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add quantized Fully Convolutional Network model #877
Conversation
Job PR-877-1 is done. |
metric.update(targets, outputs) | ||
pixAcc, mIoU = metric.get() | ||
tbar.set_description( 'pixAcc: %.4f, mIoU: %.4f' % (pixAcc, mIoU)) | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what will happen if not eval only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the current script, args.eval=False
is only required for test
mode, which will convert the input RGB images into segmented images as outputs. However, there are still incompatible issues for INT8 models and I'll try to fix them.
return args | ||
|
||
|
||
def test(args, model): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also add a dummy benchmark like ssd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add test case in test_modelzoo.py. Once #863 fix mkldnn ci, quantized fcn will be test automatically.
@@ -0,0 +1,172 @@ | |||
import os |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you rename test.py
to eval_segmentation.py
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add the quantization to the original test.py
?
If it is not possible, please change keep the orginal test.py
and create a seperate file. Do not deleate existing files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your comments. Will keep original test.py
and add support for quantized model in this script.
c39ea0c
to
e599ba0
Compare
@xinyu-intel @zhanghang1989 Updated. Please take a review again. Thanks. |
Job PR-877-3 is done. |
Job PR-877-4 is done. |
scripts/segmentation/test.py
Outdated
data = mx.gluon.utils.split_and_load(batch, ctx_list=args.ctx, batch_axis=0, even_split=False) | ||
outputs = None | ||
for x in data: | ||
output = model.forward(x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not using output=model(x)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
scripts/segmentation/test.py
Outdated
print(model) | ||
evaluator = MultiEvalModel(model, testset.num_class, ctx_list=args.ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would break the exisiting multi-size evaluation code. Could you change this script to other filename instead of overwriting test.py
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since quantized models are generated based on fixed crop_size
, so I cannot use MultiEvalModel
class here. Can we add a new function or a new script for INT8 inference? @xinyu-intel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can use a separate function ‘def quantized_test’
tests/unittests/test_model_zoo.py
Outdated
@unittest.skip("temporarily disabled to fallback to non-mkl version") | ||
@with_cpu(0) | ||
def test_quantized_fcn_models(): | ||
model_list = ['fcn_resnet101_voc_int8', 'fcn_resnet101_coco_int8-symbol'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe model_list = ['fcn_resnet101_voc_int8', 'fcn_resnet101_coco_int8']
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
@zhanghang1989 Already added a separated test function for quantized model in
|
@zhanghang1989 Could you please help take a review at this PR again? Thanks |
Could you keep the test function the same as the original to avoid introducing potential issues? |
@zhanghang1989 I will just put |
Job PR-877-7 is done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. @zhanghang1989 Can you help review again?
@zhreshold @hetong007 @Jerryzcn Do we need to put json files into GluonCV? This PR has 37K lines of files. |
My two cents on the json issue:
|
there's indeed a controvercy between portability/availability against the maintainance difficulty. However, I still prefer the model definition should remain in the gluon-cv repo, as this behavior is aligned with python definitions in My suggestion is to utilize inline python The workflow will be:
This way we will have compact int8 model definitions line by line and stored elegantly in code. |
This proposal looks good to me at this stage. |
That sounds good. |
@wuxun-zhang @xinyu-intel Please refer to above discussion for our suggested modifications. This is the first PR that comes with a significantly large json file, so please implement the proposed internal compression and decompression apis into gluoncv.utils, and include the compressed json. Once we have the APIs, please have the other existing json files compressed in another PR. Thanks! |
@hetong007 Okay |
Thanks @zhreshold for help. Now use compressed string instead of raw json file. @zhanghang1989 Please also help take a review. Thanks. |
Job PR-877-10 is done. |
gluoncv/utils/compress_json.py
Outdated
import zlib | ||
import base64 | ||
|
||
_compressed_int8_json = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel we should have the compressed string in another file or other files under model_zoo
. Utils are utils, and different from specific model definitions.
@zhreshold what do you suggest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, model_zoo
is a better place for compressed strings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wuxun-zhang Let's put the strings under gluoncv/model_zoo/quantized/
. In the next PR we can replace all the current .json
files with compressed strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix.
@wuxun-zhang Please rebase code and enable your skipped ut since #863 has been merged:) |
Job PR-877-16 is done. |
@hetong007 @zhreshold Now CI has passed. Please take a look again. Thanks. |
with warnings.catch_warnings(record=True) as w: | ||
warnings.simplefilter("always") | ||
import tempfile | ||
if "fcn" in model_name: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking for elimination of json files completely!
Looks good to me as a transition PR. |
@xinyu-intel @pengzhao-intel
This PR is to add quantized FCN models into GluonCV model-zoo. With this PR, INT8 FCN model can get more than ~4x speedup on AWS C5.12xlarge.