You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
In #2023, after building torchtext for conda-forge, I noted:
If anyone is interested, I pushed a branch with the changes that were necessary to get this to work for conda-forge.
There's several things around (not) building submodules, which are obviously not meant as patches that should be upstreamed directly (this is the part that @mthrok's proposal would cover much better by having some CMake switches like USE_SYSTEM_xxx).
There's other adaptations to our infrastructure that are not necessarily suited to this repository (yet), but perhaps someone is interested in taking those and banging them into shape. Not sure I have much time to do it myself, but anyone's free to take my patches (and I've signed the contributor license agreement already for other meta projects, so that should be fine).
@Nayef211 then asked if I can open an issue for this, so here goes.
The patches currently in the branch serve the following purposes:
h-vetinari@420cbd5 - in conda-forge we explicitly want to use the already-packaged libraries for this, not the vendored ones. This just does what we need, but is not meant to be upstreamed. @mthrok has a more comprehensive proposal for how to do this (though one single USE_SYSTEM_LIBS switch would be enough for us).
h-vetinari@60d8122 - general infrastructure work that needs to happen in conda-forge, i.e. nothing can get installed in a local build folder, everything needs to go into the various subfolders of the environment $PREFIX. It would be nice to respect e.g. CMAKE_PREFIX_PATH / CMAKE_INSTALL_PREFIX if it is specified. We also want to use Ninja universally, which for some reason didn't work when I specified CMAKE_GENERATOR, so I just removed the offending switch.
h-vetinari@12ec329 - these deps are not necessary for installing the package, but only for running the package. Pip is less good with this distinction, but in conda-forge, we try to differentiate between the two (because other things flow from that, especially when ABI-relevant packages are involved).
h-vetinari@cc58392 - this pairs with the commit further up; pytorch has been similarly installed into the respective site-packages folder, and needs to be loaded from there.
h-vetinari@a2e6933 - this function call fails if there's no git around. It fits into the larger picture of USE_SYSTEM_LIBS in that it should be completely disabled if no third-party libs are needed.
h-vetinari@37c5c5e - abseil's ABI depends on the C++ standard version used to compile, and since torchtext is exposed to abseil's ABI through sentencepiece, we need to match what we use elsewhere in conda-forge (more background).
h-vetinari@cbfb2cb - there seems to be some built-in assumption that the integration tests are called from within their own folder. I ended up calling pytest test/integration_tests, and this then runs into an issue that the path generated by download_from_url does not match what the test wants to clean up. Make those paths match.
In #2023, after building torchtext for conda-forge, I noted:
@Nayef211 then asked if I can open an issue for this, so here goes.
The patches currently in the branch serve the following purposes:
USE_SYSTEM_LIBS
switch would be enough for us).$PREFIX
. It would be nice to respect e.g.CMAKE_PREFIX_PATH
/CMAKE_INSTALL_PREFIX
if it is specified. We also want to use Ninja universally, which for some reason didn't work when I specifiedCMAKE_GENERATOR
, so I just removed the offending switch.site-packages
folder, and needs to be loaded from there.git
around. It fits into the larger picture ofUSE_SYSTEM_LIBS
in that it should be completely disabled if no third-party libs are needed.pytest test/integration_tests
, and this then runs into an issue that the path generated bydownload_from_url
does not match what the test wants to clean up. Make those paths match.Originally posted by @h-vetinari in #2023 (comment)
The text was updated successfully, but these errors were encountered: