Add support for png decoding on linux #1881

r-zenine · 2020-02-13T06:19:12Z

As discussed here, This PR add's c++ operators to decode png.

Thanks

r-zenine · 2020-02-13T07:38:52Z

It's still a WIP, I have some weird issues with the search paths.

r-zenine · 2020-02-13T15:29:30Z

There first PR is done. The failures are due to pipelines timing out. I don't know how to fix that (already tried with and without ninja ). What could we do ?

Thanks

cpuhrsch · 2020-02-13T15:58:59Z

Hello @r-zenine,

Thanks for splitting this out!

Master is currently timing out as well, I imagine it's because our builds are starting to take too long. Could you try increasing the timeout? Especially with submodule libraries that are being built from source we're increasing the build times yet again. You could also build in parallel via the -j or --parallel flags of cmake. While Ninja causes an OOM, cmake might not.

On another note, let's make zlib a submodule as well. This way we're not increasing the number of dependencies. I thinks this is the main repository. Another comment on submodules: Since you can choose the commit it might make sense to pin it to a recent release for now, if you haven't done that already. That way we should run into less risk of a dependency bug.

Christian

codecov-io · 2020-02-13T20:39:50Z

Codecov Report

❗ No coverage uploaded for pull request base (master@b6f28ec). Click here to learn what that means.
The diff coverage is 0%.

@@           Coverage Diff            @@
##             master   #1881   +/-   ##
========================================
  Coverage          ?   0.43%           
========================================
  Files             ?      93           
  Lines             ?    7418           
  Branches          ?    1119           
========================================
  Hits              ?      32           
  Misses            ?    7378           
  Partials          ?       8

Impacted Files	Coverage Δ
torchvision/io/image.py	`0% <0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b6f28ec...75f4130. Read the comment docs.

r-zenine · 2020-02-13T21:52:48Z

Hi, @cpuhrsch,

Hope you are doing well!

I have added zlib as a vendored dependency and pinned the versions for both libpng and zlib.

I increased the timeout for both circleci (30 minutes) and travis (1h).

There is one last pipeline that is failing. The unit test for ops in failing on windows. I tried to rebase from master in the hope of getting a fix for that with no success.

Have a good day!

Let me know if there is anything else left to do!

cpuhrsch · 2020-02-13T22:05:00Z

That Windows build is a mystery to me. Paging Dr. Windows @peterjc123 to take a look if he has some time.

cpuhrsch

Overall this looks fine to me. We'll just need to figure out the Windows error.

peterjc123 · 2020-02-14T04:13:06Z

Well, the test failures are not related to this PR. Looks like sth is breaking at the PyTorch side.

cpuhrsch · 2020-02-14T04:21:09Z

Ok, let's wait for the next nightly and try again, since the PyTorch repo is running clear.

test/test_image.py

r-zenine · 2020-02-17T09:59:55Z

Hi @cpuhrsch,

Please do not merge this PR. While working on the jpeg one. I figured out that even tough this approach works for now, it is not stricly what we wanted.

Here is what is happening:
We are building the extension code code against vendored versions of libpng and zlib. However, the code is running against the system wide dynamic libraries. It works because libpng and zlib are ubiquitous in linux systems.

As in torch, we would need to ship the .so in package using data_files or package_data arguments in the setup function. My struggle now is that when I do this, the code fails because the dynamic library is not found.

There is an option runtime_library_dir the might help. But there is little documentation about it.
Do you happen to know how it's done in pytorch ?

Thanks

cpuhrsch

Ok, thanks for pointing this out. Let's working on actually making this link statically.

setup.py

r-zenine · 2020-02-17T20:28:40Z

Hi @cpuhrsch ,

I have pushed a version with libpng and zlib linked dynamically and shipped with the package.
The windows pipeline is failing without any useful error message. I will try to investigate further.

What do you think of the approach ? Do we still try to link everything statically?

have a good day !

cpuhrsch · 2020-02-17T20:38:59Z

Hello @r-zenine,

Yes, ideally we link everything statically. Shipping dynamic libraries can be a nightmare and in particular building against a library that has dynamic dependencies can lead to lots of conflicts. If we can build something statically it's always preferred and if it seems possible we should try until it works.

Thanks,
Christian

r-zenine · 2020-02-17T21:34:23Z

hi @cpuhrsch ,

It works ! We are linking everything statically !
Thanks for pushing hard for this !

The only thing that remains is the windows error that has nothing to do with the PR !

Have a good day

r-zenine · 2020-02-18T19:40:49Z

Hi @cpuhrsch ,

What are the next steps ? Anything else i have to do ?

Have a good day !

cpuhrsch · 2020-02-18T20:03:30Z

Great! Thanks again for your work! I'll take a closer look at the Windows build and also a few other things related to this PR setup. This might take a few days / week.

r-zenine · 2020-02-18T20:33:39Z

Hi @cpuhrsch ,

Let me work on MacOs support on the same PR. I'll try to push that on thursday. I will let you handle the windows pipeline and will get back to work on the png support.

Thanks a lot for the help !

r-zenine · 2020-02-20T07:07:32Z

Hi @cpuhrsch ,

I just added macos support in the build. Tests for image are passing everywhere but other pipelines are failing because of other tests. It's probably due to something that happened with pytorch nightly.

Have a good day !!

r-zenine · 2020-02-25T09:59:16Z

Hi @cpuhrsch ,

I think the conda pipelines are failing because of a linking issue. There is a weird warning in the build :

WARNING (torchvision,lib/python3.5/site-packages/torchvision/_C.so): did not find - or even know where to look for: /lib64/libm.so.6
WARNING (torchvision,lib/python3.5/site-packages/torchvision/_C.so): did not find - or even know where to look for: /lib64/libc.so.6
WARNING (torchvision,lib/python3.5/site-packages/torchvision/_C.so): did not find - or even know where to look for: $RPATH/libtorch_python.so
WARNING (torchvision,lib/python3.5/site-packages/torchvision/_C.so): did not find - or even know where to look for: /lib64/ld-linux-x86-64.so.2
WARNING (torchvision,lib/python3.5/site-packages/torchvision/_C.so): did not find - or even know where to look for: /lib64/libpthread.so.0
WARNING (torchvision,lib/python3.5/site-packages/torchvision/_C.so): Needed DSO lib/libgcc_s.so.1 found in [u'libgcc-ng']
WARNING (torchvision,lib/python3.5/site-packages/torchvision/_C.so): .. but [u'libgcc-ng'] not in reqs/run, (i.e. it is overlinking) (likely) or a missing dependency (less likely)
WARNING (torchvision,lib/python3.5/site-packages/torchvision/_C.so): did not find - or even know where to look for: $RPATH/libpng16.so.16
WARNING (torchvision,lib/python3.5/site-packages/torchvision/_C.so): did not find - or even know where to look for: $RPATH/libtorch_cpu.so
   INFO (torchvision,lib/python3.5/site-packages/torchvision/_C.so): Needed DSO lib/libcudart.so.10.0 found in defaults::cudatoolkit-10.0.130-0
WARNING (torchvision,lib/python3.5/site-packages/torchvision/_C.so): did not find - or even know where to look for: $RPATH/libc10_cuda.so
WARNING (torchvision,lib/python3.5/site-packages/torchvision/_C.so): Needed DSO lib/libstdc++.so.6 found in [u'libstdcxx-ng']
WARNING (torchvision,lib/python3.5/site-packages/torchvision/_C.so): .. but [u'libstdcxx-ng'] not in reqs/run, (i.e. it is overlinking) (likely) or a missing dependency (less likely)
WARNING (torchvision,lib/python3.5/site-packages/torchvision/_C.so): did not find - or even know where to look for: $RPATH/libtorch.so
WARNING (torchvision,lib/python3.5/site-packages/torchvision/_C.so): did not find - or even know where to look for: $RPATH/libtorch_cuda.so
WARNING (torchvision,lib/python3.5/site-packages/torchvision/_C.so): did not find - or even know where to look for: $RPATH/libc10.so
WARNING (torchvision): dso library package defaults::python-3.5.6-hc3d631a_0 in requirements/run but it is not used (i.e. it is overdepending or perhaps statically linked? If that is what you want then add it to `build/ignore_run_exports`)

Do you have any idea where it is coming from ? Do we have that on master, or is it specific to this PR ?

Thanks

cpuhrsch · 2020-02-28T22:24:06Z

Hello @r-zenine,

We'll need to wait on @fmassa for some insight here. He's currently working on a hi-pri deadline so this will take a bit longer than expected.

Is this working locally for you? You could also try installing this within a docker container (if your environment allows you to set that up) to get local feedback on the linking behavior etc.

In particular, if you take a look at the CI jobs you can see that they pull in the "pytorch/conda-cuda" container. This could be worthwhile looking into so you can get a faster turnaround time on experiments.

Thanks,
Christian

r-zenine · 2020-03-04T10:28:24Z

Hi @fmassa , @cpuhrsch

Here is a status of what's going on, I tried to statically link the libraries (it works with wheel not conda and I could not figure out a fix with conda despite dedicating many hours to it). I also tried dynamically linking against libpng and shipping the library with the package ( like what pytorch does for it's dependencies). It's seems to be working on most pipeline. There is a bug with the linking in macos conda that I still need to figure out.

If we go that way, it might make updating the windows pipelines easier.

Thoughts ?

- Pipelines are timing out, increasing to 30 minutes. - Vendors zlib and add's libpng as a dependency. - Ship libpng.so and zlib.so with package on linux

- Installs cmake on macos - Fix conda not found. in macos_wheel - Update $PATH on macOs_wheel for cmake - activate tests for macos

This reverts commit f7bc8a4.

andfoy · 2020-06-15T19:41:54Z

@r-zenine, thanks for your contribution! We've migrated your commits to #2301, so we will close this one

r-zenine requested a review from cpuhrsch February 13, 2020 06:19

r-zenine changed the title ~~Add's support for png decoding on linux~~ WIP: Add's support for png decoding on linux Feb 13, 2020

r-zenine force-pushed the read-png-linux branch 5 times, most recently from caeac7e to d102d39 Compare February 13, 2020 14:42

r-zenine force-pushed the read-png-linux branch from dd6aac7 to 6018845 Compare February 13, 2020 20:41

cpuhrsch approved these changes Feb 14, 2020

View reviewed changes

r-zenine changed the title ~~WIP: Add's support for png decoding on linux~~ Add's support for png decoding on linux Feb 14, 2020

cpuhrsch reviewed Feb 14, 2020

View reviewed changes

test/test_image.py Show resolved Hide resolved

cpuhrsch requested changes Feb 17, 2020

View reviewed changes

setup.py Outdated Show resolved Hide resolved

r-zenine force-pushed the read-png-linux branch from 4ca3367 to 7e94649 Compare February 17, 2020 19:42

r-zenine changed the title ~~Add's support for png decoding on linux~~ Add's support for png decoding on linux and macos Feb 20, 2020

cpuhrsch requested a review from fmassa February 24, 2020 23:10

cpuhrsch added the triage review label Feb 24, 2020

cpuhrsch changed the title ~~Add's support for png decoding on linux and macos~~ Add support for png decoding on linux and macos Feb 25, 2020

r-zenine force-pushed the read-png-linux branch from 5df6861 to 0033a3a Compare March 4, 2020 06:54

r-zenine force-pushed the read-png-linux branch from 0a37692 to e736e78 Compare March 6, 2020 07:15

r-zenine changed the title ~~Add support for png decoding on linux and macos~~ Add support for png decoding on linux Mar 8, 2020

r-zenine added 8 commits March 8, 2020 15:40

Add support for png decoding on linux

978d41e

- Pipelines are timing out, increasing to 30 minutes. - Vendors zlib and add's libpng as a dependency. - Ship libpng.so and zlib.so with package on linux

Fix windows pipeline error

daba9d0

Statically linking zlib and libpng

e758748

Add's support for macos

7ee1d02

- Installs cmake on macos - Fix conda not found. in macos_wheel - Update $PATH on macOs_wheel for cmake - activate tests for macos

Improve errors handling and fix unit tests

7d757ac

Attempt to fix conda builds

08a70af

Revert "Statically linking zlib and libpng"

e264be1

This reverts commit f7bc8a4.

drops mac support for now

75f4130

r-zenine force-pushed the read-png-linux branch from b8c2efe to 75f4130 Compare March 8, 2020 14:40

This was referenced Jun 5, 2020

Package libjpeg and libpng within torchvision #2291

Closed

PR: Add libpng and libjpeg-turbo requirement into conda recipe #2301

Merged

andfoy closed this Jun 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for png decoding on linux #1881

Add support for png decoding on linux #1881

r-zenine commented Feb 13, 2020 •

edited

Loading

r-zenine commented Feb 13, 2020

r-zenine commented Feb 13, 2020

cpuhrsch commented Feb 13, 2020

codecov-io commented Feb 13, 2020 •

edited

Loading

r-zenine commented Feb 13, 2020

cpuhrsch commented Feb 13, 2020

cpuhrsch left a comment

peterjc123 commented Feb 14, 2020 •

edited

Loading

cpuhrsch commented Feb 14, 2020 •

edited

Loading

r-zenine commented Feb 17, 2020 •

edited

Loading

cpuhrsch left a comment

r-zenine commented Feb 17, 2020

cpuhrsch commented Feb 17, 2020

r-zenine commented Feb 17, 2020

r-zenine commented Feb 18, 2020

cpuhrsch commented Feb 18, 2020

r-zenine commented Feb 18, 2020

r-zenine commented Feb 20, 2020

r-zenine commented Feb 25, 2020

cpuhrsch commented Feb 28, 2020

r-zenine commented Mar 4, 2020

andfoy commented Jun 15, 2020 •

edited

Loading

Add support for png decoding on linux #1881

Add support for png decoding on linux #1881

Conversation

r-zenine commented Feb 13, 2020 • edited Loading

r-zenine commented Feb 13, 2020

r-zenine commented Feb 13, 2020

cpuhrsch commented Feb 13, 2020

codecov-io commented Feb 13, 2020 • edited Loading

Codecov Report

r-zenine commented Feb 13, 2020

cpuhrsch commented Feb 13, 2020

cpuhrsch left a comment

Choose a reason for hiding this comment

peterjc123 commented Feb 14, 2020 • edited Loading

cpuhrsch commented Feb 14, 2020 • edited Loading

r-zenine commented Feb 17, 2020 • edited Loading

cpuhrsch left a comment

Choose a reason for hiding this comment

r-zenine commented Feb 17, 2020

cpuhrsch commented Feb 17, 2020

r-zenine commented Feb 17, 2020

r-zenine commented Feb 18, 2020

cpuhrsch commented Feb 18, 2020

r-zenine commented Feb 18, 2020

r-zenine commented Feb 20, 2020

r-zenine commented Feb 25, 2020

cpuhrsch commented Feb 28, 2020

r-zenine commented Mar 4, 2020

andfoy commented Jun 15, 2020 • edited Loading

r-zenine commented Feb 13, 2020 •

edited

Loading

codecov-io commented Feb 13, 2020 •

edited

Loading

peterjc123 commented Feb 14, 2020 •

edited

Loading

cpuhrsch commented Feb 14, 2020 •

edited

Loading

r-zenine commented Feb 17, 2020 •

edited

Loading

andfoy commented Jun 15, 2020 •

edited

Loading