tch-rs
tch-rs copied to clipboard
LaurentMazare
Make `torch-sys/download-libtorch` an opt-in feature instead of default
In the case of using tch as a sub-dependency of other crates, disabling the default feature of downloading libtorch when depending on any of them becomes impossible.
According to this comment, features that are marked as default should be so only if it's something that will be used in most cases, not as a way to mark something optional.
I feel that the download bit is actually fairly commonly used and not having it in the default feature would make it harder for new users to set things up.
Could you say a bit more about the use case where this is a problem to you? Maybe it would be possible for you to set the LIBTORCH environment variable to get around the automated download?
It's a major stability- and security-concern for us to have this functionality compiled into production code and/or our build pipeline.
We're maintaining a quite strict cargo deny config to make sure we're only using/depending on libraries that we absolutely need, which is essential for a large enough project.
If we can make sure the feature flag is not set, the concerns will essentially be gone after compilation is done.
It will also reduce compile times to not compile crates like curl that we won't have any use for.
I might be overly pedantic here but having a build step that will download a ~1 GB lib if you're not careful doesn't outweigh the convenience it might provide for new users in my opinion. Especially when it's impossible to opt out of it when using an indirect dependency, and requiring each dependency to add pass-through feature flags would also be inconvenient for those lib maintainers and users. To me, this makes it very hard to justify using this lib in a production environment.
Downloading things is known to impose side-effects. I had a non-x86 server that requires manual compilation of libtorch installed on non-standard path (That's /usr. There are reasons we cannot install files there.). Every time I start my editor, the rust-analyzer starts to download the tar anyway, blocking the editor for minutes. I would rather prefer an explicit error to remind me that LIBTORCH is not set.
Sorry this took me a long while but I now agree that downloading libtorch by default was likely a mistake (I recently wrote some bindings for xla (xla-rs) and not having this behavior there was a lot nicer. Also the build script has recently been updated to support using libtorch from the local python environment which makes setting things up even simpler. I've crafted an updated PR for this #707 that also takes care of fixing the CI, handling the conflicts since this PR was created, and provide some detailed error message for when the feature is not enabled.
For future readers, this PR has been superseded by #707 and has been merged.
This PR can be closed.
