-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Add TOSA support #1521
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
base: main
Are you sure you want to change the base?
Add TOSA support #1521
Conversation
Added support for TOSA. Change-Id: I9db557cbee42bb3950198692cd8f2f5aab7c8c47
115d90e to
13c0251
Compare
lutzroeder
left a comment
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.
@fresve-arm thank you for the pull request. Trying to understand why this format is needed given it is almost identical to dozens of other formats and already seems to bring versioning complexity? For example, asking an AI agent it will recommend this is a high maintenance and low value effort.
| @@ -1,3 +1,4 @@ | |||
| // SPDX-FileCopyrightText: Copyright 2025 Arm Limited and/or its affiliates <[email protected]> | |||
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.
Remove
| return value & ((1 << bits) - 1); | ||
| }; | ||
|
|
||
| DataView.prototype.getInt48 = DataView.prototype.getInt48 || function(offset, littleEndian) { |
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 does some heavy allocation. Robot suggested this approach:
DataView.prototype.getInt48 = DataView.prototype.getInt48 ||
function(offset, littleEndian) {
let value;
if (littleEndian) {
const low = this.getUint32(offset, true);
const high = this.getUint16(offset + 4, true);
value = low + high * 0x100000000;
} else {
const high = this.getUint16(offset, false);
const low = this.getUint32(offset + 2, false);
value = high * 0x100000000 + low;
}
// Sign extension: if bit 47 is set, subtract 2^48
if (value >= 0x800000000000) {
value -= 0x1000000000000;
}
return value;
};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.
True, it does allocate a small buffer if there is less than 8 bytes of data available. I'll have a look at the suggested alternative approach.
|
|
||
| export const tosa = {}; | ||
|
|
||
| tosa.Version = class Version { |
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.
Fold into tosa.js.
| @@ -0,0 +1,310 @@ | |||
| // SPDX-FileCopyrightText: Copyright 2025 Arm Limited and/or its affiliates <[email protected]> | |||
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.
Remove
| MIT License | ||
|
|
||
| Copyright (c) Lutz Roeder | ||
| Copyright 2025 Arm Limited and/or its affiliates <[email protected]> |
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.
Remove
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 understand from the rest of the codebase this is the only place with a copyright notice. I also respect that you may not want to have added copyrights to the license file. Would you consider adding an AUTHORS or CONTRIBUTORS file for listing copyrights of contributors?
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.
The app is MIT licensed and aims to keep licensing and contribution requirements simple and consistent. For small contributions, especially those mainly serving a business need, adding copyright files introduces unnecessary complexity.
Agree that contributors should be credited. AUTHORS files have their own issues, and the current merge style isn’t ideal either. So while we look for a better long-term option, how about adding a simple // Contributed by Fredrik Svedberg to tosa.js?
| @@ -1,3 +1,4 @@ | |||
| // SPDX-FileCopyrightText: Copyright 2025 Arm Limited and/or its affiliates <[email protected]> | |||
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.
Remove
| 'tf', | ||
| 'uff', | ||
| 'xmodel' | ||
| 'xmodel', |
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.
Sorted alphabetically.
| } | ||
|
|
||
| schema() { | ||
| for TOSA_VERSION in ${TOSA_VERSIONS[@]} |
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.
There should be a single tosa-schema.js and tosa-metadata.json file. For metadata this can be accomplished by adding version tags similar to ONNX and other formats. Breaking schemas instead of incrementally versioning them tends to be a bad practice as it will make maintenance costly and breaking user expectations.
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'm willing to drop pre-1.0 support if this would ease the acceptance of this PR?
With 1.0 and forwards there should not be any breaking changes to the schema, which would reduce the need for maintenance.
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.
Sounds good. mslite.js went through a similar cycle, e.g. copy the same approach to informing the user.
| for TOSA_VERSION in ${TOSA_VERSIONS[@]} | ||
| do | ||
| bold "tosa metadata ${TOSA_VERSION}" | ||
| python3 ./tools/tosa_metadata.py --xml ./third_party/tosa/tosa_${TOSA_VERSION}.xml > ./source/tosa-metadata-${TOSA_VERSION}.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.
Structure is third_party/source/tosa/.... When using curl folder structure should match paths in the repo in case curl later needs to be replaced with git clone.
| #!/usr/bin/env python3 | ||
| # SPDX-FileCopyrightText: Copyright 2025 Arm Limited and/or its affiliates <[email protected]> | ||
|
|
||
| """Parsing of tosa.xml to tosa_metadata.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.
Since this doesn't have any Python specific dependencies please converted to JavaScript using ../source/xml.js.
396dc74 to
6c1732b
Compare
Some motivation why this format was created can be found in the introduction of the specification https://www.mlplatform.org/tosa/tosa_spec_1_0_1.html#_introduction |
da94ec5 to
166f0a0
Compare
c78cb29 to
ec17c2e
Compare
Added support for TOSA.
See: https://www.mlplatform.org/tosa/tosa_spec.html