-
Notifications
You must be signed in to change notification settings - Fork 88
Port C++ code to rely on torch stable ABI API. #1188
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
base: main
Are you sure you want to change the base?
Conversation
| class DummyDeviceInterface : public DeviceInterface { | ||
| public: | ||
| DummyDeviceInterface(const torch::Device& device) : DeviceInterface(device) {} | ||
| DummyDeviceInterface(const StableDevice& device) : DeviceInterface(device) {} |
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.
@dvrogozh , heads up that this is something we might want to do in the near future.
The general context is that we want to rely on torch stable ABI so that we (or you) don't have to synchronize torchcodec releases with torch releases 6X a year.
I think it means that the third-party extensions will have to pass-in "Stable" types - you might still be able to use the non-stable ones internally as long as you convert them.
Let me know if you have any concern about that.
Note that the PR is in a very, very WIP state at the moment. Plenty of things are likely to change. But this is the direction we're heading to.
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.
@NicolasHug , thank your for heads up. I will start looking into the story, but "stable" sounds very appealing. Once PR will be in a closer to merge stage I would love to give it a try on my side (create respective PR for xpu and see how it goes).
| # Set PyTorch stable ABI target version to 2.11 for consumers. | ||
| # This ensures third-party code uses stable ABI types compatible with | ||
| # the library. Format: 0x MAJOR(2) MINOR(2) PATCH(2) TWEAK(2) zeros(8) | ||
| INTERFACE_COMPILE_DEFINITIONS "TORCH_TARGET_VERSION=0x020b000000000000" |
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.
TODO: I'm not sure we'll want to enforce that on third-parties?
CC @dvrogozh
| ) | ||
| from .ops import ( | ||
| _add_video_stream, | ||
| _destroy_decoder, |
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.
TODO: there's no way we want that. Aparently this is because from_blob doesn't support deleters? See comment in custom_ops.cpp.
| // For Decoding: enables users to pass in the entire video or audio as bytes. | ||
| // Our read and seek functions then traverse the bytes in memory. | ||
| class AVIOFromTensorContext : public AVIOContextHolder { | ||
| class TORCHCODEC_API AVIOFromTensorContext : public AVIOContextHolder { |
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.
TODO: understand why that's needed. Ideally it shouldn't.
| // Enable CUDA-specific functions in PyTorch stable ABI headers | ||
| #define USE_CUDA 1 |
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.
TODO: uh??
| if (!device_.has_index()) { | ||
| int resolvedIndex = getDeviceIndex(device_); | ||
| device_.set_index(static_cast<StableDeviceIndex>(resolvedIndex)); | ||
| } |
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.
TODO above: this was causing a hang - investigate.
| target_include_directories(${library_name} | ||
| BEFORE PRIVATE | ||
| ${pybind11_INCLUDE_DIRS} | ||
| ) |
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.
TODO investigate above.
| "-Wno-attributes" | ||
| ) | ||
| endif() | ||
|
|
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.
TODO on all these
| // For a tensor of shape [numChannels, numSamples], we need to calculate | ||
| // offsets | ||
| outputBuffers[i] = static_cast<uint8_t*>(lastSamples.mutable_data_ptr()) + | ||
| i * numRemainingSamples * sizeof(float); |
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.
TODO waaat
| // Note: impl_abstract_pystub is not available in stable ABI. | ||
| // The Python stubs are handled via Python-side registration instead. |
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.
TODO what?
| // exits. Note: This map is not thread-safe; concurrent decoder | ||
| // creation/destruction from multiple threads is not supported. | ||
| std::unordered_map<SingleStreamDecoder*, std::unique_ptr<SingleStreamDecoder>> | ||
| g_decoder_registry; |
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.
TODO: we reaaaally want to avoid the new logic. Can we add deleter support to the stable ABI from_blob?
| "_add_video_stream(Tensor(a!) decoder, *, int? num_threads=None, str? dimension_order=None, int? stream_index=None, str device=\"cpu\", str device_variant=\"ffmpeg\", str transform_specs=\"\", Tensor? custom_frame_mappings_all_frames=None, Tensor? custom_frame_mappings_is_key_frame=None, Tensor? custom_frame_mappings_duration=None, str? color_conversion_library=None) -> ()"); | ||
| m.def( | ||
| "add_video_stream(Tensor(a!) decoder, *, int? num_threads=None, str? dimension_order=None, int? stream_index=None, str device=\"cpu\", str device_variant=\"ffmpeg\", str transform_specs=\"\", (Tensor, Tensor, Tensor)? custom_frame_mappings=None) -> ()"); | ||
| "add_video_stream(Tensor(a!) decoder, *, int? num_threads=None, str? dimension_order=None, int? stream_index=None, str device=\"cpu\", str device_variant=\"ffmpeg\", str transform_specs=\"\", Tensor? custom_frame_mappings_all_frames=None, Tensor? custom_frame_mappings_is_key_frame=None, Tensor? custom_frame_mappings_duration=None) -> ()"); |
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.
TODO: can't pass tuples anymore?
| StableTensor& decoder, | ||
| std::optional<int64_t> num_threads = std::nullopt, | ||
| std::optional<std::string_view> dimension_order = std::nullopt, | ||
| std::optional<std::string> dimension_order = std::nullopt, |
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.
TODO we went from string_view to string everywhere, is that OK? Is that needed?
|
|
||
| // Allocate output tensor | ||
| // Note: Stable ABI's from_blob doesn't support custom deleters, | ||
| // so we allocate and copy the data instead |
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.
TODO: nope
| # Register atexit handler to clear all decoders before Python shuts down. | ||
| # This prevents hangs when decoders holding file-like objects are destroyed | ||
| # during static destruction (after Python has started shutting down). | ||
| atexit.register(_clear_all_decoders) |
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.
TODO: nope
No description provided.