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

Plans for weight converter? Common interface. #383

Open
constantinpape opened this issue Apr 29, 2024 · 2 comments
Open

Plans for weight converter? Common interface. #383

constantinpape opened this issue Apr 29, 2024 · 2 comments

Comments

@constantinpape
Copy link
Contributor

There have been some recent changes to the weight converters and it seems not everything there is up-to-date.

In weight_converter/torch/_torchscript the function converts the weights and returns the corresponding weight description:
https://github.com/bioimage-io/core-bioimage-io-python/blob/main/bioimageio/core/weight_converter/torch/_torchscript.py#L98

In weight_converter/torch/_onnx the function name implies that the weights would directly be added to the model, but that does not seem to happen in the function. Note that this initially followed the same design as in the torchscript converter when I implemented this.
See https://github.com/bioimage-io/core-bioimage-io-python/blob/main/bioimageio/core/weight_converter/torch/_onnx.py

In weight_converter/keras/_tensorflow the function is similar to the torchscript one above, but it does not return the weight description.
See https://github.com/bioimage-io/core-bioimage-io-python/blob/main/bioimageio/core/weight_converter/keras/_tensorflow.py#L101

It would be good to agree to a common syntax for this functionality and outline a plan to update this and make it usable again.
I would prefer to follow the syntax in the torchscript file; adding the weight directly to the model could be added as a different function that makes use of the conversion functions internally.

@FynnBe
Copy link
Member

FynnBe commented Apr 30, 2024

Totally! These converter submodules should only need minor updates (with the option to add additional converters).
Providing both functionalities as you suggest makes sense to me 👍
Anyone interested in contributing is welcome to tackle this. I would do my best to provide answers to any questions that may arise.

@FynnBe
Copy link
Member

FynnBe commented Apr 30, 2024

@bioimage-io/spec-dev anyone interested in taking a look?

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

No branches or pull requests

2 participants