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 rten-convert #53

Merged
merged 7 commits into from
Feb 16, 2024
Merged

Add rten-convert #53

merged 7 commits into from
Feb 16, 2024

Conversation

igor-yusupov
Copy link
Contributor

@igor-yusupov igor-yusupov commented Feb 9, 2024

This implements #45.

@robertknight
Copy link
Owner

Hello - can you remove the converter module and generated schema from the previous location under tools/ in the same PR. Thanks!

@igor-yusupov
Copy link
Contributor Author

Done :)

@robertknight
Copy link
Owner

There is a rule in the project's Makefile that will need updating too, so that editing src/schema.fbs and then running make all puts the generated Python code in the new location.

@igor-yusupov
Copy link
Contributor Author

Should I update Readme file, specifically the places where model conversion is specified? I assume you are planning to add rten-convert to pypi, aren't you?

@robertknight
Copy link
Owner

Should I update Readme file, specifically the places where model conversion is specified? I assume you are planning to add rten-convert to pypi, aren't you?

Yes please, and yes, I plan to upload it to PyPI.

@igor-yusupov
Copy link
Contributor Author

I have updated readme files and comments in example code. For now I have specified installation from the repository, but once you add the package to PyPi, will need to correct the installation of the package from PyPI.

 - Update dependencies of the `all` target

 - Update the output directory for `schema_generated.py`
@@ -293,7 +293,8 @@ fn print_input_output_list(model: &Model, node_ids: &[NodeId]) {
/// generated inputs.
///
/// ```
/// tools/convert-onnx.py model.onnx output.rten
/// pip install -e rten-convert
Copy link
Owner

Choose a reason for hiding this comment

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

In the instructions in examples and other tools I think it will make sense to assume the user has followed the basic installation steps in the main README, so we don't need to duplicate the instructions for installing rten-convert in each example. This was already assumed to be the case for the dependencies of tools/convert-onnx.py.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed unnecessary installation references

@robertknight robertknight merged commit ab489a7 into robertknight:main Feb 16, 2024
2 checks passed
@robertknight
Copy link
Owner

Thanks for this. I will publish the tool to PyPI as part of the next RTen release.

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