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

Keep support for model v1 within the rust client #593

Open
yaniv5678 opened this issue Jul 24, 2024 · 9 comments
Open

Keep support for model v1 within the rust client #593

yaniv5678 opened this issue Jul 24, 2024 · 9 comments
Labels
rust Pull requests that update Rust code

Comments

@yaniv5678
Copy link

Hi,
Is there any possibility to add support for running v1 version of the model?
Maybe in a Cargo.toml feature.

Thanks!

@invernizzi invernizzi added the rust Pull requests that update Rust code label Aug 8, 2024
@reyammer
Copy link
Collaborator

Hello,

what would the use case for this? Ensure backward compatibility? Or you noticed specific issues with v2? Additional context would help!

@yaniv5678
Copy link
Author

As far as I seen, v2 is slower than v1 as it is a larger model, so we prefer using v1 for working with data in scale :)

@reyammer
Copy link
Collaborator

thanks for the feedback, this is very useful! /cc @ia0 @invernizzi

We do have a smaller version of our v2 models (https://github.com/google/magika/tree/main/assets/models/fast_v2_1), it can be easily used by the python module (by pointing model_dir to it), but it's not integrated yet with the rust codebase (and it's currently not so trivial to do so).

We'll discuss internally on how to approach this. In the meantime, please let us know if you have additional context to share. For example: it seems you are integrating the magika rust cli within an existing pipeline... in which language is this pipeline written? If, for example, the pipeline is written in python, the python module would be the wait to go: most of rust's performance improvements are about avoiding the initial one-off starting time, and after things are loaded in python, the inference time rust vs python should be roughly the same.

@reyammer reyammer changed the title Model v1 Keep support for model v1 within the rust client Sep 19, 2024
@era
Copy link

era commented Jan 8, 2025

Hi @reyammer, I also noticed the difference in performance from v1 to v2. I enabled fast_v2_1 by changing the symlink and making the hardcoded model.rs match the config.min.json.

It seems to work fine :).

So I'm guessing the code changes needed to support the fast_v2_1 are done?

Anyway, commenting here in the hopes that this helps people who want a faster model than the v2.

@reyammer
Copy link
Collaborator

So I'm guessing the code changes needed to support the fast_v2_1 are done?

Yes, the rust client supports all v2 models; and what you did is the way to use the fast model.

We are thinking on how to allow the rust client to pick which model to use and how to prioritize this over other features.

In addition, we may soon have another model that is significantly faster with pretty much the same accuracy (still WIP / in testing at the moment), so maybe this problem will solve itself soon.

@yaniv5678
Copy link
Author

Yeah @era that's what I've also done, it works well indeed :)

@reyammer note that on windows machines the symlink stuff does not work well, I think it's related to something with git.
The content of the link file in a windows machine is the relative path to the symlink itself, instead of being a proper windows link.

And regarding the new model - sounds really cool that it's gonna be even faster! Waiting for this.

@ia0
Copy link
Member

ia0 commented Jan 11, 2025

note that on windows machines the symlink stuff does not work well

Indeed, we don't support development on Windows, only the published library and binary are cross-platform. However, this can be easily fixed by duplicating the rust/lib/src/model.onnx file with a CI script to make sure it's in sync, and completely avoiding rust/gen/model by using a constant in the code.

@reyammer do we want to support development on Windows? I can make a PR.

@era
Copy link

era commented Jan 13, 2025

However, this can be easily fixed by duplicating the rust/lib/src/model.onnx file with a CI script to make sure it's in sync, and completely avoiding rust/gen/model by using a constant in the code.

@reyammer @ia0 maybe a similar idea could be used to support different models? Gate the different models behind a crate feature and the needed changes. Here is a quick and dirty example of what I mean: https://github.com/google/magika/compare/main...era:magika:main?expand=1 (in case the link does not work this and this commits)

The only thing is that features in Rust must be additive, so although unlikely it would need to support people who compiled it with multiple models: https://doc.rust-lang.org/cargo/reference/features.html#feature-unification

Anyway, thank you all for the hard work and looking forward to the faster model 🚀

@ia0
Copy link
Member

ia0 commented Jan 13, 2025

Gate the different models behind a crate feature

Yes, that's one option, but we want to possibly go further (depending on user need). The current design I have in mind is:

  • Make the library generic over the actual model.
  • Support "static dispatch": The library ships with a set of models backed in the source. You have compile-time guarantees on which content types the model can return.
  • Support "dynamic dispatch": You can load a model from a file. It doesn't need to be shipped with the library.
  • Gate which models end up in the static set with a cargo feature for each model. (For those who care about code size.)
  • Gate the dynamic dispatch behind a cargo feature. (Same rationale.)

(Note that cargo features are additive with this option.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rust Pull requests that update Rust code
Projects
None yet
Development

No branches or pull requests

5 participants