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.

Sign up for GitHub

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

Jump to bottom

[Non-breaking] Bundling models into the package itself. #811

Merged

mazenchami merged 17 commits into infinitered:master from haZya:bundling-models

Mar 5, 2024

Contributor

haZya commented Jan 14, 2024 •

edited

Loading

Pull Request

Related issue

Fixes #779, #807, #808

What does this PR do?

This bundles all 3 models into the package itself. This also adds support for both ESM and CJS, allowing bundlers like Webpack to tree-shake unused modules.

This gets rid of the reliance on the currently hosted model on cloudfront.net and s3 bucket.

Added the ability to the load() to specify which model to use between the 3 available options, "MobileNetV2" | "MobileNetV2Mid" | "InceptionV3". Defaults to: "MobileNetV2".
Updated the examples to reflect the changes.
Updated the documentation to reflect the changes.
Added editorconfig to improve consistency.
Refactored some files and folders to add more coherence.

Caveats

This converts the model weights from binary to base64 since .bin files cannot be directly imported into JS. As a result, the bundle size increases for each model by approximately 1.33 times (33%). i.e. The "MobileNetV2" model bundled into the package is 3.5MB instead of 2.6MB for hosted binary files. This means it will have slightly higher loading times than using the binary files directly. If this is a concern, users can host the models on their own servers and use the URL as usual in the load(), as described in the documentation's Host your own model section. This will only make a difference if the model is loaded every time (without caching) on the client-side browser since on the server-side, you'd only be loading the model once at the start of the server.

Network analysis

Bundle analysis

haZya added 11 commits

January 7, 2024 22:49


          Bundle the models into the package itself.

a0f6afd

This bundles all 3 models into the package itself. This also adds support for both `ESM` and `CJS` allowing bundlers like `Webpack` to `tree-shake` the unused models.

Added the ability to the `load()` to specify which model to use between the 3 available options, `"MobileNetV2"` | `"MobileNetMid"` | `"InceptionV3"`.


          refactor: models folder

56e2853


          feat: Add model bundling support to browserified version

86d69eb

Use browserify to convert model files to .js files.

+ refactor: reorder package.json scripts.
+ refactor: rename tsconfig files.
+ refactor: rename `MobileNetMid` to `MobileNetV2Mid`.


          Update minimal_demo with bundled models

f60cb13


          refactor: example folder

644dbdb

+ refactor: rename example folder to examples.
+ Update manual_testing example with bundled models.


          Update .npmignore


          Rename example to examples for all references of the directory

f193d6b


          refactor: move to the dedicated scripts folder

27c62db


          fix: bundle imports in tests

2813e93

+ test: add pretest and posttest scripts.
+ refactor: reorder package.json.


          docs: Update README.md

c559881

+ Update usage with bundled models.
+ Refactors.


          Add @haZya as a contributor

f05f91b

Contributor Author

haZya commented Jan 14, 2024

Netlify build seems to be failing due to the folder rename from example to examples. If this is not desired we can revert that change.


          Update .npmignore

a83a0d0

Member

GantMan commented Feb 6, 2024 •

edited

Loading

BTW, I love where this is going!

We're reviewing this soon.

Contributor

mazenchami commented Feb 7, 2024

@haZya thank you very much fro putting this PR together. I was wondering if you could resolve conflicts based on the two PRs I just got merged in (#817 & #819). Once you get those addressed, I'll review your PR and see if we can get this in the next major release version!

haZya added 2 commits

February 8, 2024 13:12


          Merge branch 'master' into 'bundling-models' and resolve merge conflicts

1840f88


          test: Update and refactor the tests

fdb414f

+ Remove node-fetch polyfill from devDependencies as it is no longer being used.
+ Add to contributions.

Contributor Author

haZya commented Feb 8, 2024

Hi @mazenchami @GantMan

I've updated this PR to align with the latest changes in the main branch and refactored a few things as well. I have also added a few changes to the tests to reflect the changes. Everything should be stable. Please take your time and review the changes.

The only check that is failing is the renaming of the example folder to examples. I think this makes sense as it contains more than one example. If there are any areas you'd like to be reverted or if you need further clarification, please let me know.

Cheers.

Contributor

mazenchami commented Feb 8, 2024

Hi @mazenchami @GantMan

I've updated this PR to align with the latest changes in the main branch and refactored a few things as well. I have also added a few changes to the tests to reflect the changes. Everything should be stable. Please take your time and review the changes.

The only check that is failing is the renaming of the example folder to examples. I think this makes sense as it contains more than one example. If there are any areas you'd like to be reverted or if you need further clarification, please let me know.

Cheers.

@haZya thanks for making the updates. regarding the failing deploy check to Netlify, we'll be making the fix this morning along with merging my PR #820 which should also reduce the number of files you have.

I haven't had the opportunity to get to fully review your PR but I have two comments:

it looks like your linter made some auto corrections to the README.md file. if you don't mind, can you run the following two commands and see if any changes are made: yarn contributors:generate && yarn toc
this is a fix we really want to get in to this package since there are a lot of issues around it, as you mentioned in your description. there is one question i want to ask. it looks like on each reload, the model will be reloaded. that to me isn't optimal. what're your thoughts around having a sort of "load model" sort of function? that way when the user loads their app, its quick and then when they get to the right spot, they can load the model and proceed with what is needed? thoughts around this?

also, not sure if you're on the IR Slack Community group, but please, feel free to join and DM me there and we can chat around this further if you'd like!

Contributor

mazenchami commented Feb 8, 2024

@haZya my PR #820 was just merged. please pull and resolved conflicts as it should help with the deploy part!

Member

GantMan commented Feb 8, 2024

@haZya - thanks for chilling with us while we do some much-needed maintenance to this repo! I think we're nearing the end of our merge conflicts, and then we'll dive in and evaluate your PR extensively!


          Merge branch 'master' into bundling-models

d76fa38

Contributor Author

haZya commented Feb 9, 2024

Hey @GantMan Thanks for the heads up!

Contributor Author

haZya commented Feb 9, 2024 •

edited

Loading

Thanks for the update @mazenchami.

For the first point you've mentioned about the README.md file, could you kindly show me where you saw the linting issues? I've added changes to the README to reflect some of the changes in the PR and ran yarn contributors:generate && yarn toc. I can't see any linting issues against the existing README. FYI I've added a .editorconfig file with some default linting rules to make the code more consistent since there was no consistency before. Some lines had semicolons at the end while some didn't, some lines used double quotes while some others used single quotes etc. Please feel free to adjust the formatting rules if the ones I've added are not desired.
Apologies if I didn't exactly get what you meant here. But I think you're probably mistaken about how the models would load in this PR. Let me explain.
- When the user loads the model for the first time in a frontend application by calling load() it starts downloading the specified model. The user can call the load() anywhere in their application and that is when it will download the model and load it. It does not download the model until the load() is called.
- Since this PR also adds support for ESM it will only download the model that the user is using and the other 2 models will not be downloaded unless they switch the model like with the nsfw_demo where there is a select to load another model. It will only download the specified model in the load() and bundlers like Webpack will tree-shake the unused models therefore no additional network overhead.
- Once the model is downloaded for the first time, it will be cached by the browser automatically. So if the user were to reload the page it would not redownload the same model. However, if the user were to do a hard reload (Ctrl + F5), it would clear the browser cache and redownload the model.

Correct me if I'm wrong, but I believe this is the same behaviour as the latest version of NSFWJS.

Still, I agree we could do better here for the nsfw_demo example. For instance, right now it loads the model on the main thread. Due to JavaScript’s single-threaded nature, even though the loadModel function is asynchronous, the loading of the TensorFlow model is a computationally intensive task that can block the main thread, causing the React frontend to appear stuck. This is especially noticeable if the connection speed is slow or when loading the largest InceptionV3 model. This is not an issue introduced by this PR as this is the existing behaviour when loading the model even in its current version. However, as a solution, we could consider using a Web Worker to load the model in a background thread, freeing up the main thread for tasks like UI updates.

I believe this is the best way to load the model on the front end without blocking the main thread. Typically it would be ideal to load the model on a server and access it via API instead of loading it on each client browser. But if the user wants to load the model on the client side, I think using a web worker would be the best approach.

Furthermore, I have expanded the Caching section in the docs to explain how caching the model on the browser using localStorage or indexeddb could help reuse the model without redownloading even if the user were to do a hard reload (Ctrl + F5). This could also be added to the nsfw_demo to further speed up loading after the initial load.

I'd be willing to do a PR for this as well on this weekend or the next if you are interested. Please let me know.

PS: I've also joined the IR Slack Community. Feel free to do any follow-ups on there if needed. Thanks.

haZya and others added 2 commits

February 10, 2024 15:56


          Merge branch 'master' into bundling-models

773095d

+ Resolved merge conflicts in package.json.


          Merge branch 'master' into bundling-models

3f0a5b6

mazenchami approved these changes

View reviewed changes

Contributor

mazenchami left a comment

Great work on this PR! This will get merged and deployed soon! 🎉 🚀

.all-contributorsrc Show resolved Hide resolved

mazenchami merged commit 39aa4cb into infinitered:master

5 checks passed

ghnp5 reviewed

View reviewed changes

README.md Show resolved Hide resolved

Member

GantMan commented Mar 5, 2024

Hey @haZya - thank you so much for your contribution! We owe you a drink if we can ever meet you in person!

Contributor Author

haZya commented Mar 5, 2024

Hey @GantMan @mazenchami

Thank you very much for your time and support in reviewing this PR.

I'll probably follow up with another PR later for the nsft_demo with a Web Worker to spread the model loading into a separate thread. Unfortunately, I won't be able to find the time for it right now. So it will take a little while. I'd try to do it when I find some free time again. 😅

Cheers.

Contributor

mazenchami commented Mar 5, 2024

Hey @GantMan @mazenchami

Thank you very much for your time and support in reviewing this PR.

I'll probably follow up with another PR later for the nsft_demo with a Web Worker to spread the model loading into a separate thread. Unfortunately, I won't be able to find the time for it right now. So it will take a little while. I'd try to do it when I find some free time again. 😅

Cheers.

@haZya absolutely, any time! and thank YOU for the contribution! feel free to tag me as a reviewer when you get it up and i'll be happy to take a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet