training: avoid copying std::future; make ThreadPool::run move-friendly (bug fix) #97
Conversation
|
This is likely a follow-up of #96 . Don't worry about At this point, I mostly need the reviews of @kevinjzhang and @Victor-C-Zhang , since they own the code being modified. |
| #include <unordered_set> | ||
| #include "openzl/common/a1cbor_helpers.h" | ||
| #include "openzl/common/allocation.h" | ||
| #include <future> |
There was a problem hiding this comment.
@kevinjzhang
this PR proposes adding <future> to this unit.
Does that sound correct to you ?
There was a problem hiding this comment.
This include seems fine to me considering they are already used. I'm surprised this is necessary tho because a transitive dependency thread_pool.h includes future
tools/training/utils/thread_pool.h
Outdated
| #include <queue> | ||
| #include <thread> | ||
| #include <vector> | ||
| #include <mutex> |
There was a problem hiding this comment.
@kevinjzhang @Victor-C-Zhang
This PR adds quite a number of dependencies to thread_pool.h.
What do you think ?
There was a problem hiding this comment.
These are all part of libstdc++ so I think it's fine. I think most of these should be transitive dependencies anyways.
tools/training/utils/thread_pool.h
Outdated
| std::future<ReturnType> result = task->get_future(); | ||
| template <class F, class... Args> | ||
| auto run(F&& f, Args&&... args) | ||
| -> std::future<std::invoke_result_t<std::decay_t<F>, std::decay_t<Args>...>> { |
There was a problem hiding this comment.
@terrelln or @felixhandte could you take a look at this?
Summary: Adds missing STL headers required for Windows/clang-cl builds: - `<string>` for `std::to_string` in `Logger.h` - `<numeric>` for `std::accumulate`, `std::iota` in test and training files - `<stdexcept>` for `std::runtime_error` in `InputSet*.cpp` - `<array>` for `std::array` in `test_functionGraphs.cpp` - `<optional>` for `std::optional` in `InputSetBuilder.h` - `<chrono>` for `std::chrono::high_resolution_clock` in trainer files - Added MSVC detection (`_MSC_VER`) for IEEE 754 floating-point support in `portability.h` These headers are implicitly included via transitive dependencies on GCC/Clang but MSVC requires explicit includes. **No runtime or performance impact** - purely compile-time fixes. Fixes #95 **Note:** I have also validated that #97 (std::future move-only fix) works correctly on my Windows setup. ## Type of Change - [x] Bug fix (non-breaking change which fixes an issue) Pull Request resolved: #299 Test Plan: Built the project cmake -S . -B build -G "Visual Studio 14 2015" -T ClangCL cmake --build build --config Release build is now successful The only error is for libzstd_shared.vcxproj (the shared/DLL version of zstd): ```fatal error RC1012: mismatched parenthesis : missing '``` This is a resource compiler issue with Clang headers when building the shared library. But this is non-blocking After build successful, subsequent fix is about passing test, correcting ```std::future``` using PR from #97 and std::uniform distribution #301 ### Test Configuration - **Compiler:** clang-cl (ClangCL toolset) - **Generator:** Visual Studio 14 2015 with ClangCL toolset (`-T ClangCL`) - **Build type:** Release - **Platform:** Windows 11 x64 Reviewed By: Cyan4973 Differential Revision: D89547335 Pulled By: zhiningli fbshipit-source-id: 9fef41317cd73e9505c4fbef59b6041a5ad2a848
|
@zhiningli merged this pull request in 3924936. |
6 participants
Fixes #95
bug:
Windows/clang-cl build of training tools failed with:
error: call to deleted constructor of 'std::future<...>'
caused by a lambda capturing std::vector<std::future<...>> by value while MSVC’s std::packaged_task copies the callable internally (copy move-only type)
Fix:
tools/training/utils/thread_pool.h: implement run(...) via perfect-forwarding + std::apply; capture callable/args by move
tools/training/clustering/compression_utils.cpp: capture futures via std::shared_ptr<std::vector<std::future>> initialized with std::move; iterate over *futures_ptr