-
Notifications
You must be signed in to change notification settings - Fork 7k
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
Deprecate the C++ vision::models namespace #4375
Conversation
73154eb
to
f5a8c54
Compare
f5a8c54
to
f6c9a95
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @datumbox , I have a few remarks / questions
They are not currently being maintained.
Somewhat related is #3992. It shouldn't take too much effort to put it back on track but it might not be needed after all
They represent an old attempt to support Vision models in C++. This was superseded by JIT. All of the models we propose to remove are fully jit-traceable and jit-scriptable.
It seems however that relying on jit for this involves a slightly more cumbersome workflow: namely, one has to first export a model from Python so that it can then be used in C++. This means that you can the exported models also needs to be shipped as part of the binaries I guess. What do we think about this?
Also, is there a similar effort on the pytorch side? Quoting @fmassa 's #3992 (comment)
it would be good to know who is the current maintainer of the C++ API of PyTorch
Finally, with torchscript being currently unbundled to different other packages, will there be a different alternative for this in the future? (say torch::deploy or something else)
@NicolasHug Thanks for the comments. Unfortunately, maintaining the namespace requires significant more effort than one would think. The reason, most of the newer models are missing is because they rely on operators that are only available in Python. Moreover offering a C++ API for only the old classification models makes very little sense (segmentation and detection models are all missing). Please note that this initiative is coordinated with Francisco and discussed with Greg and Brian. If you have more concerns about this, let's chat offline. |
Thanks @datumbox , I would still be interested in having my questions above answered. Perhaps there is a document that I could read? |
@NicolasHug Sounds good. We can gather references related. to the above and you are welcome to lead the efforts to make them presentable publicly so that the community can benefit. |
Thanks for the proposal @datumbox but since I'm not involved in this work, I don't think it should be up to me |
Hi, To address some points brought up by @NicolasHug comments:
The torchscript and C++ APIs are two different ways of getting your model into C++, and they target different users.
I believe this is fine and what is done by the rest of the community. If you imagine that you have your binary that runs your application, and that this binary takes a serialized model as an argument, then shipping this extra file is very reasonable (and we don't need to recompile the app if we switch models).
Yes, I would expect that we will have a 3rd option to enable models to run in C++, which is similar to the torchscript solution but running entirely with Python. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @datumbox !
After discussing with Vasilis, we decided to change the message to indicate that we are not actively maintaining the C++ models, but we will keep them around.
Thank you for addressing my questions @fmassa |
Summary: * Add deprecation warnings on vision::models * Change the C++ example. * Chage readme. * Update deprecation warning. Reviewed By: kazhang Differential Revision: D30898331 fbshipit-source-id: 64edd30d726469111dd33821b4d0befc25c9b4dd
We should consider dropping the C++
vision::models
namespace for the following reasons:This PR deprecates the entire namespace by adding warnings. It also updates the code examples to show how to integrate models using JIT.