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

metadata parsing #1490

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

Conversation

heabeounMKTO
Copy link
Contributor

attempt at issue 1006

  • load meta data from onnx model path.

@kali
Copy link
Collaborator

kali commented Aug 1, 2024

Hello, thanks for your interest in tract.

What are we trying to achieve here ? If we want to integrate ONNX metadata with tract, there are a couple issue I think is worth considering:

  • put the ONNX metadata in tract models metadata somehow to avoid introducing a separate API and concepts
  • this code assumes a file and a path, while models can be loaded from buffers as well (it is useful to load models from http endpoints in CI settings)
  • mmap is not portable to wasm

I think we can kill this three birds with one stone actually, if we handle metadata parsing and rewrapping in tract metadata inside onnx model loading functions.

Alternatively, maybe what you are trying to achieve is a metadata access that does not require reading the entire file...

WDYT ?

@heabeounMKTO
Copy link
Contributor Author

i want to be able to access onnx metadata through the onnx model, similar to how we can access it through python. so i think i'll wrap the parsing of metadata in the onnx loading function, thanks!

@heabeounMKTO
Copy link
Contributor Author

ive put a new loading function inside of the Onnx struct , since i'm learning rust , i'm not sure if this is the correct way to do it , please kindly advise :) @kali

@kali
Copy link
Collaborator

kali commented Aug 5, 2024

OK, so this is not what I had in mind. What I was thinking was:
1/ load the ONNX metadata in parse_with_template, refactoring the operator set version extraction which is actually part of the metadata
2/ inject them in the "properties" of the template model (or after building the graph)

WDYT ?

@heabeounMKTO
Copy link
Contributor Author

i actually thought about doing it that way too but i thought it was too complex for me to implement it , now that you mentioned it , i will take it up as a challenge and re-implement it in that way , thanks for the advice ! :)

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