Skip to content
This repository was archived by the owner on Sep 10, 2025. It is now read-only.

Conversation

@mthrok
Copy link
Contributor

@mthrok mthrok commented Jun 23, 2022

Context:

TorchText uses dual-binding (PyBind11 and TorchBind) to make custom operations available in Python.
The both binding eventually calls the same implementation contained in libtorchtext.so.
The ones bound via PyBind11 (the ones in torchtext._torchtext) calls into libtorchtext.so.

Untitled drawing

This means that libtorchtext.so has to make the symbols (APIs) used by torchtext._torchtext visible.

However, the default visibility of symbols in shared libraries are different in Windows.
On Windows all the symbols are by default hidden. To work around this, we use CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS to expose all the symbols.
There is an upper limit of visible symbols one library fine can contain, and this can be problematic in the future.
(Although it is unlikely that torchtext will hit the limit, unless it introduces custom CUDA kernels.)

A better approach is to selectively mark the symbols that should be visible as visible.

Summary of the change set

This commit introduces TORCHTEXT_API macro which annotates functions with proper visibility.
The core logic was taken from https://github.com/pytorch/pytorch/blob/bcc02769bef1d7b89bec724223284958b7c5b564/c10/macros/Export.h

The behavior is as follow;

For non-Windows: It is always __attribute__((__visibility__("default")))
For Windows:
If the header is included from the compilation unit of libtorchtext, then it resolves to __declspec(dllexport). otherwise it resolves to __declspec(dllimport).

This allows to remove CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS.

@mthrok mthrok requested review from Nayef211 and parmeet June 23, 2022 23:10
@mthrok mthrok marked this pull request as ready for review June 23, 2022 23:10
Copy link
Contributor

@Nayef211 Nayef211 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @mthrok for introducing the change to add correct visibility for Windows systems.

@mthrok
Copy link
Contributor Author

mthrok commented Jul 5, 2022

Test for fb internal https://www.internalfb.com/diff/D37475369

@mthrok mthrok merged commit e1c7bc6 into pytorch:main Jul 7, 2022
@mthrok mthrok deleted the public-api branch July 7, 2022 21:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants