Skip to content
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

Do not depends on unused libraries for libtiff #785

Merged

Conversation

gigony
Copy link
Contributor

@gigony gigony commented Sep 26, 2024

Libraries such as jbig, libwebp-base, and zlib are not used for the functionality of cuCIM.
These codec libraries are linked to libtiff only when available at build time.

This patch explicitly disables external codecs and removes unnecessary dependencies from the dependencies.yaml.

With this patch, I can confirm that they are not requested at all.

--  Support for external codecs:
--   ZLIB support:                       OFF (requested)  (availability)
--   Pixar log-format algorithm:         OFF (requested) FALSE (availability)
--   JPEG support:                       OFF (requested) TRUE (availability)
--   Old JPEG support:                   OFF (requested) TRUE (availability)
--   JPEG 8/12 bit dual mode:            OFF (requested) FALSE (availability)
--   ISO JBIG support:                   OFF (requested) FALSE (availability)
--   LZMA2 support:                      OFF (requested)  (availability)
--   ZSTD support:                       OFF (requested)  (availability)
--   WEBP support:                       OFF (requested) FALSE (availability)

@gigony gigony added non-breaking Introduces a non-breaking change maintenance labels Sep 26, 2024
@gigony gigony self-assigned this Sep 26, 2024
@gigony gigony requested review from a team as code owners September 26, 2024 17:36
@gigony gigony added the improvement Improves an existing functionality label Sep 26, 2024
@gigony gigony force-pushed the remove_unnecessary_dependencies branch from 3ac5a54 to 58f183a Compare September 26, 2024 17:40
@jakirkham
Copy link
Member

Thanks Gigon! 🙏

Can we please drop these as well?

- jbig
- libwebp-base

- jbig
- xz
- zlib
- zstd

- xz
- zlib
- zstd

@gigony
Copy link
Contributor Author

gigony commented Sep 26, 2024

Thanks Gigon! 🙏

Can we please drop these as well?

- jbig
- libwebp-base

- jbig
- xz
- zlib
- zstd

- xz
- zlib
- zstd

Thanks @jakirkham!
I missed those. Appreciate the reminder!

@gigony gigony force-pushed the remove_unnecessary_dependencies branch from 58f183a to c108199 Compare September 26, 2024 17:44
@@ -92,10 +87,6 @@ requirements:
- libnvjpeg
{% endif %}
- {{ pin_compatible('libwebp-base', max_pin='x.x') }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we dropped libwebp-base above, this can also be dropped

Suggested change
- {{ pin_compatible('libwebp-base', max_pin='x.x') }}

Copy link
Contributor Author

@gigony gigony Sep 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jakirkham !
Also removed the other line containing libwebp-base in the same file.

Signed-off-by: Gigon Bae <gbae@nvidia.com>
@gigony gigony force-pushed the remove_unnecessary_dependencies branch from c108199 to 303981a Compare September 26, 2024 18:29
@jakirkham
Copy link
Member

Thanks Gigon! 🙏

Are we able to use the Conda libtiff package? If so, we could drop this build step altogether (simplifying this further)

Copy link
Member

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found one reference in docs that should be updated (xz and zlib should be removed from it):

Otherwise, you may need to install dependencies (such as zlib, xz, yasm) through your OS's package manager (`apt`, `yum`, and so on).

Otherwise, this looks good to me!

I won't merge this until you and @jakirkham come to some agreement on whether to go further and directly depend on conda-forge's libtiff (#785 (comment)). My perspective on that... it should be documented in an issue as a potential follow-up and not done here. I expect it'd take some fairly-involved design work to answer questions like:

  • what is the ongoing amount of effort to require the source build of libtiff?
  • how do wheels get their libtiff dependency?
  • does the set of codecs supported by the conda-forge package include everything cucim needs? Are we ok with it pulling in more, unnecessary things (for example, it does depend on libwebp, xz, and zstd)

Signed-off-by: Gigon Bae <gbae@nvidia.com>
@jakirkham jakirkham added this to the v24.10.00 milestone Oct 1, 2024
@jakirkham
Copy link
Member

@jameslamb could you please take another look? 🙂

@jakirkham
Copy link
Member

Are we able to use the Conda libtiff package? If so, we could drop this build step altogether (simplifying this further)

Discussed this point with Gigon earlier, we decided to leave this task for PR ( #753 ), which is already doing that work

@gigony
Copy link
Contributor Author

gigony commented Oct 1, 2024

I found one reference in docs that should be updated (xz and zlib should be removed from it):

Otherwise, you may need to install dependencies (such as zlib, xz, yasm) through your OS's package manager (`apt`, `yum`, and so on).

Otherwise, this looks good to me!

I won't merge this until you and @jakirkham come to some agreement on whether to go further and directly depend on conda-forge's libtiff (#785 (comment)). My perspective on that... it should be documented in an issue as a potential follow-up and not done here. I expect it'd take some fairly-involved design work to answer questions like:

  • what is the ongoing amount of effort to require the source build of libtiff?
  • how do wheels get their libtiff dependency?
  • does the set of codecs supported by the conda-forge package include everything cucim needs? Are we ok with it pulling in more, unnecessary things (for example, it does depend on libwebp, xz, and zstd)

Thanks @jameslamb for your thoughtful comments!

Removed the wording 'xz' and 'zlib' in CONTRIBUTING.md file.

Regarding 'directly depend on conda-forge's libtiff, there is one-going work for Devendering CMake dependencies (#686, #753)

image

  • what is the ongoing amount of effort to require the source build of libtiff?

I will continue working on it to use conda-forge's libraries, including libtiff.

  • how do wheels get their libtiff dependency?

In the current implementation, the source code of libtiff is built as a static library and linked to cucim.kit.cuslide@24.xx.00.so, so no additional third-party library dependencies are required from the shared library in the Python wheel.

I'm not entirely sure about that.
I can see that conda-forge's libtiff package (https://anaconda.org/conda-forge/libtiff/files) includes a static library (libtiff.a).
I may end up static linking all the dependent static libraries (https://github.com/conda-forge/libtiff-feedstock/blob/main/recipe/meta.yaml#L39C3-L46C37), or vendoring those dependent libraries in the wheel file, which might increase the size of the wheel.

Let me consider those aspects as I continue to work on #753.

Thanks!

Copy link
Member

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed, this looks good to me and I supported merging it. Thanks for considering my questions about libtiff, I'm glad that won't be part of the scope for this PR.

@jakirkham
Copy link
Member

/merge

@rapids-bot rapids-bot bot merged commit 289e340 into rapidsai:branch-24.10 Oct 2, 2024
45 checks passed
@jakirkham
Copy link
Member

Thanks Gigon and James! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing functionality maintenance non-breaking Introduces a non-breaking change
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants