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

Acuity support #614

Closed
wants to merge 1 commit into from
Closed

Acuity support #614

wants to merge 1 commit into from

Conversation

hawk081
Copy link

@hawk081 hawk081 commented Oct 13, 2020

Hi,

This patch enables the support of Acuity models, please help to review the patch, any suggestions are welcome.

Acuity is an AI framework that can convert the models of other framework(such as tensorflow, tflite, caffe, onnx, etc) to Acuity models, then generate high performance code for embedded platforms.

.json file is the Acuity model file which contains the model structure, and .data file will be loaded for weights and biases, .quantize file is for quanzation parameters. the output tensors' shape of each layer will be inferred if the input shape(s) are provied in model files.

Some popular models converted by Acuity can be found here: https://verisilicon.github.io/acuity-models/

Thanks.

#657

@lutzroeder lutzroeder changed the title Acuity models support Acuity support Oct 14, 2020
@lutzroeder lutzroeder force-pushed the main branch 3 times, most recently from c2fbdd5 to a80ba14 Compare October 17, 2020 23:18
@lutzroeder lutzroeder force-pushed the main branch 6 times, most recently from deafe0a to dee687a Compare October 31, 2020 02:53
@hawk081
Copy link
Author

hawk081 commented Nov 9, 2020

Hi Lutz,

Is there anything need to be modified ?

Thanks.

Copy link
Owner

@lutzroeder lutzroeder left a comment

Choose a reason for hiding this comment

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

  1. There are multiple similar formats for deep learning models already. Can Acuity use an existing format instead of creating another duplicate format?
  2. jsyaml should not be checked in as obfuscated code. For JSON, Protocol Buffers and FlatBuffers there are custom implementation using base.TextDecoder. A similar approach should be followed for YAML.
  3. Using JSON and YAML and Pickle (3 different standards) for a single model is a fragile design. Ideally there should be no YAML dependency.

test/models.json Outdated
{
"type": "acuity",
"target": "inception_v1.json",
"source": "https://raw.githubusercontent.com/VeriSilicon/acuity-models/master/models/inception_v1/inception_v1.json",
Copy link
Owner

Choose a reason for hiding this comment

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

The file is sorted alphabetically by type. This is only testing the JSON code, not the YAML and binary readers. Please add a more comprehensive sample.

@hawk081
Copy link
Author

hawk081 commented Nov 11, 2020

Thanks!

  1. Current format of .json, .data and .quantize are used in our production, It is difficult to migrate to other formats.
  2. We will follow the JSON to implement a YAML parser.
  3. For some historical reasons, we have to use these 3 files to describe the model for now.

@lutzroeder
Copy link
Owner

lutzroeder commented Nov 28, 2020

Current format of .json, .data and .quantize are used in our production, It is difficult to migrate to other formats.

It will only get more difficult over time. This is an overcomplicated design with lots of moving parts. Better to fix it early on while you still can. Ideally you should consider what value the framework actually provides and if it does need yet another new file format or can just use an existing one.

@lutzroeder lutzroeder force-pushed the main branch 8 times, most recently from 59e7f16 to ad04a3a Compare December 9, 2020 03:01
@hawk081
Copy link
Author

hawk081 commented Dec 9, 2020

Thanks for your suggestions.

I will remove the code of loading .quantize and .data, let netron just handle the model structure.

@lutzroeder lutzroeder force-pushed the main branch 8 times, most recently from acf818a to 404b5f4 Compare December 13, 2020 23:55
@lutzroeder lutzroeder force-pushed the main branch 2 times, most recently from 64dd724 to f11c219 Compare December 20, 2020 05:18
@hawk081 hawk081 closed this Dec 21, 2020
@hawk081 hawk081 reopened this Dec 21, 2020
Signed-off-by: Jiang Best <[email protected]>
lutzroeder added a commit that referenced this pull request Dec 22, 2020
lutzroeder added a commit that referenced this pull request Dec 22, 2020
@lutzroeder
Copy link
Owner

Why is this format needed? There are a dozen other deep learning formats that are more or less identical.

@lutzroeder lutzroeder closed this Dec 22, 2020
@hawk081
Copy link
Author

hawk081 commented Dec 22, 2020

Thanks! When we started the project at 2016, there weren't suitable formats for us. so ...

lutzroeder added a commit that referenced this pull request Aug 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants