Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
PR: Add libpng and libjpeg-turbo requirement into conda recipe #2301
PR: Add libpng and libjpeg-turbo requirement into conda recipe #2301
Changes from 101 commits
2b78943
23255aa
006ab0c
7b9ec24
f97a9f0
ac6d26e
b14912e
770cea5
0861b80
a42a029
b7a19ea
1afde4d
386fd5b
2b5c469
0341aa5
721e5e3
2186d68
b80fb08
3d153f0
36b0a8f
9d14d9e
852a289
3e86f49
021e767
e734175
58c6524
b9295c1
02fa9d9
6c757d4
c207eab
a1aa2e6
34fc7d6
3ed2044
eaaf658
3edae46
c17202e
741f855
a46f503
4bad033
b419fc1
83eff79
eb2846f
3563ef3
347383f
51f6b48
ad00442
4d82283
32b2207
9a5aefe
a44c3b5
dfcde68
d8d46d6
eea8552
8c7dc31
ee8148a
059fa42
0ed1af6
11d1a7a
3bb65ba
c334d7e
5650e59
7894836
2da51d4
78455ae
a0a383d
f143e2c
95cc941
1c9270a
ee388e5
462ed6c
989db57
6e9ad0e
dd43bcd
e542e48
b5fa45e
3e90556
4e09af0
c2e9bf3
8ac335e
2adb87e
44826a7
1e830ea
37c889b
3cca366
2a6ff9f
b89b349
264cb74
a0ce4ca
bd752aa
5259757
3013247
9d8b1b5
7c3ec51
158eec8
269d8e5
273dc1a
cfc7c75
d32a5f0
051425b
3bc7323
6b87895
af61f94
123fd3f
831749c
558b0cb
8b2f507
b560227
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
is it now safe to use
conda-forge
as well? At some point we had issues with it.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.
It seems only libturbojpeg is being pulled from there, so all other packages are being pulled from defaults or the main PyTorch channel
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.
Could you explain when / how these conflicts materialize?
Could that lead to segfaults when the user imports torchvision and PIL?
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.
The conflict here is that we're substituting the libjpeg library, which is used primarily by Pillow, however this may not affect us as the ABI between jpegturbo and libjpeg is the same AFAIK.
Most Linux distributions distinguish between libturbojpeg (the library itself) and libjpeg-turbo (the libjpeg version using turbojpeg), thus, they do not have this problem, as we link against libturbojpeg, rather than against libjpeg-turbo.
In conda-forge, we have this conflict because the recipe for libjpeg-turbo produces both the turbo flavored libjpeg and libturbojpeg. I spoke to @isuruf, one of the maintainers in conda-forge, and he told me that they are working towards a solution in conda-forge/conda-forge.github.io#673, but right now there are no alternatives to this conflict.
I guess we are not having any problem with this conflicting installation, as the tests are passing. However, we should encourage users to install PyTorch and torchvision on a separate environment, so we prevent other errors that could be caused as part of this conflict and we're not aware of
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.
Hum, this makes me a bit worried, because AFAIK we don't have tests running on OSX, only Linux and Windows (we do compile on OSX though).
So if there is a problem happening in OSX we wouldn't be able to see it in CI.
Do you think that, if we were to use libjpeg API, everything would be safer?
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.
Aren't they running on the
binary_macos_conda_*_pyxx
CircleCI pipelines?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.
Oh ok, yeah, we were not running tests for OSX with wheels, only conda.
But still, do you think if we were to use libjpeg API it would make things safer?
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.
I think right now we don't have much trouble on our setup, as we are linking against libturbojpeg and not libjpeg directly. If we were compilling against libjpeg, then we would be in trouble. Right now the only conflict that we have is the one on conda-forge.
A provisional solution would be compilling libturbojpeg (Without libjpeg) ourselves and publish it into the conda pytorch channel until conda-forge/conda-forge.github.io#673 is fixed. What do you think about this?
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.
Is this necessary? We are able to package the
_C.so
without this.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.
This is necessary to pack libpng/turbojpeg into the wheel distributions