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

Add mindir format support. #1227

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

Ethan-Chen-plus
Copy link

🤗👋Hi! We have added our .mindir format. Please review our commit.

@lutzroeder
Copy link
Owner

lutzroeder commented Feb 18, 2024

@Ethan-Chen-plus can you provide some context:

  • What is mindir, what problem is it solving and for how many users?
  • Why is a new format needed given there are many similar formats?
  • Create a script in ./tools following the patterns used for other frameworks to recreate the -proto file.
  • Add a test to ./test/models.json and share an index of 50+ existing files in this format.
  • Run npm run validate

@Ethan-Chen-plus
Copy link
Author

PS I:\netron> npm run validate
eslint
pylint
test
gguf/phi-2.Q2_K.gguf
onnx/candy.onnx
keras/1151.4.keras
coreml/Exermote.mlmodel
pytorch/alexnet.ptl
pytorch/DCGAN2.pt
tf/conv-layers.pb.zip
tflite/mobilenet_v1_0.75_160_quantized.tflite
tflite/squeezenet.tflite

I have updated my code, please review.
@lutzroeder

@lutzroeder
Copy link
Owner

@Ethan-Chen-plus can you provide some context:

  • What is mindir, what problem is it solving and for how many users?
  • Why is a new format needed given there are many similar formats?
  • Please rebase to latest main.
  • The ./tools scripts should generate regenerate to files from the original .proto. When changing the format or generator for -proto.js or -metadata.json these files should automatically get regenerated from the original source when running ./tools/mindir sync schema metadata.

@Ethan-Chen-plus
Copy link
Author

@lutzroeder Hello.👋 I noticed that eslint failed. Could you please tell me which config file eslint is using? I'll make the necessary fixes based on that config file. Thank you!

@lutzroeder lutzroeder force-pushed the main branch 4 times, most recently from 952b6ed to 2781faa Compare May 5, 2024 03:43
@Ethan-Chen-plus
Copy link
Author

Ethan-Chen-plus commented May 5, 2024

Workflow requires approval, thank you🤗

@lutzroeder lutzroeder force-pushed the main branch 3 times, most recently from d9a23ec to 5028ef6 Compare May 20, 2024 17:26
@Ethan-Chen-plus
Copy link
Author

Workflow requires approval, thank you again🤗

@lutzroeder lutzroeder force-pushed the main branch 4 times, most recently from 1a3b633 to dc51e07 Compare May 26, 2024 14:49
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.

Compared to other similar IR formats such as ...

This format looks very similar to ONNX. Instead of defining a new format, can this be accomplished by using metadata extensions in ONNX to augment with additional information.

There is also an existing MindSpore Lite format and a dozen other formats that look similar in principle. Consider a path that leverages existing formats or standards instead of creating another duplicate format that needs to be maintained.

@@ -734,7 +734,8 @@ base.Metadata = class {
'ptl', 't7',
'dlc', 'uff', 'armnn',
'mnn', 'ms', 'ncnn', 'om', 'tm', 'mge', 'tmfile', 'tnnproto', 'xmodel', 'kmodel', 'rknn',
'tar', 'zip'
'tar', 'zip',
'mindir', 'mind_ir'
Copy link
Owner

Choose a reason for hiding this comment

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

Remove for now for testing.

@@ -0,0 +1,713 @@
export const protobuf = {};
Copy link
Owner

@lutzroeder lutzroeder May 27, 2024

Choose a reason for hiding this comment

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

Rename to mindir-proto.js. Sync to latest and regenerate this file using ./tools/mindir sync schema.

@@ -0,0 +1,252 @@
syntax = "proto2";
Copy link
Owner

Choose a reason for hiding this comment

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

This file should not be added to the repro and instead downloaded to a location in ./third_party/source/mindspore/mindspore/core/proto/mind_ir.proto.

sync() {
echo "mind_ir sync"
mkdir -p "./third_party/source/mindspore/mindspore/mindir/schema/"
curl --silent --location --output "./third_party/source/mindspore/mindspore/mindir/schema/mind_ir-metadata.json" "https://raw.githubusercontent.com/Ethan-Chen-plus/mindir/main/mind_ir-metadata.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 metadata file should not be downloaded from an external location and instead be updated in place as part of ./tools/mindir metadata.

@@ -5455,6 +5455,34 @@ view.ModelFactoryService = class {
this.register('./modular', ['.maxviz']);
this.register('./cambricon', ['.cambricon']);
this.register('./weka', ['.model']);
this.register('./mslite', ['.ms']);
Copy link
Owner

@lutzroeder lutzroeder May 27, 2024

Choose a reason for hiding this comment

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

Resolve incorrect merge.

* The Model class represents a mind_ir model and contains information about the model's format, name, and graphs.
* The Graph class represents a graph within a mind_ir model and contains information about the graph's inputs, outputs, and nodes.
*/
export const protobuf = {};
Copy link
Owner

@lutzroeder lutzroeder May 27, 2024

Choose a reason for hiding this comment

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

Remove, this should not be needed.

const identifier = context.identifier;
const extension = identifier.split('.').pop().toLowerCase();
if (extension === "mindir") {
return 'mind_ir';
Copy link
Owner

@lutzroeder lutzroeder May 27, 2024

Choose a reason for hiding this comment

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

This has changed in more recent versions. Sync to latest.

}
};

mind_ir.Parameter = class {
Copy link
Owner

Choose a reason for hiding this comment

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

Renamed in latest. Parameter to Argument and Argument to Value.


constructor(message) {
super(message);
this.name = 'SnapML Model Load Error.';
Copy link
Owner

Choose a reason for hiding this comment

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

Fix error message.


constructor(context, attribute) {

this._name = attribute.name;
Copy link
Owner

Choose a reason for hiding this comment

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

Attribute has been removed in latest and replaced with using Argument to represent attribtues.

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.

None yet

2 participants