-
Notifications
You must be signed in to change notification settings - Fork 9
2397 make vt work with fmt 11 and integrate it if possible #2399
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
2397 make vt work with fmt 11 and integrate it if possible #2399
Conversation
Pipelines resultsPR tests (gcc-10, ubuntu, openmpi, no LB) Build for 9ab5ec3 (2025-02-14 00:39:06 UTC) PR tests (clang-13, alpine, mpich) Build for 9ab5ec3 (2025-02-14 00:39:06 UTC) PR tests (clang-9, ubuntu, mpich) Build for 9ab5ec3 (2025-02-14 00:39:06 UTC) PR tests (clang-12, ubuntu, mpich) Build for 9ab5ec3 (2025-02-14 00:39:06 UTC) PR tests (clang-13, ubuntu, mpich) Build for 9ab5ec3 (2025-02-14 00:39:06 UTC) PR tests (clang-11, ubuntu, mpich) Build for 9ab5ec3 (2025-02-14 00:39:06 UTC) PR tests (gcc-11, ubuntu, mpich, trace runtime, coverage) Build for 9ab5ec3 (2025-02-14 00:39:06 UTC) PR tests (gcc-12, ubuntu, mpich, verbose, kokkos) Build for 9ab5ec3 (2025-02-14 00:39:06 UTC) PR tests (intel icpx, ubuntu, mpich, verbose) Build for 9ab5ec3 (2025-02-14 00:39:06 UTC) PR tests (intel icpc, ubuntu, mpich) Build for 9ab5ec3 (2025-02-14 00:39:06 UTC) PR tests (nvidia cuda 11.2, gcc-9, ubuntu, mpich) Build for 9ab5ec3 (2025-02-14 00:39:06 UTC) PR tests (nvidia cuda 12.2.0, gcc-9, ubuntu, mpich, verbose) Build for 9ab5ec3 (2025-02-14 00:39:06 UTC) PR tests (clang-14, ubuntu, mpich, verbose) Build for 9ab5ec3 (2025-02-14 00:39:06 UTC) PR tests (gcc-9, ubuntu, mpich, zoltan) Build for 9ab5ec3 (2025-02-14 00:39:06 UTC) PR tests (clang-10, ubuntu, mpich) Build for 9ab5ec3 (2025-02-14 00:39:06 UTC) |
| }; | ||
|
|
||
| inline auto format_as(TimeTypeWrapper t) { | ||
| return to_engineering_string(t.seconds(), 5, eng_exponential, "s"); |
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.
We could avoid including EngFormat-Cpp/eng_format.hpp in the header by moving the implementation to .cpp.
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.
Looks good to me! Nice +/- ratio for vt code 👍
Git check complains:
lib/fmt/CMakeLists.txt:411: new blank line at EOF.
tests/unit/lb/test_lbargs_enum_conv.nompi.cc:166: new blank line at EOF.
9ab5ec3 to
46f7d98
Compare
8832c36 to
48b829a
Compare
48b829a to
2255888
Compare
|
Rebased to get fresh logs. edit: |
Reduced reproducer: #include <brotli/decode.h>
#include <fmt-vt/format.h>
int main(int, char**) {
BrotliDecoderState* dec_ = nullptr;
// (...)
auto error_code = BrotliDecoderGetErrorCode(dec_);
auto error_str = fmt::format("code={}", error_code);
return 0;
} |
Ok, for this we just need to cast the enum to the underlying type. I think the gcc-10 failure is the one I was looking at related to Werror. |
|
Here's the vt-tv fix: DARMA-tasking/vt-tv#145 |
740dcfd to
3ed309c
Compare
|
diff --git a/src/vt/epoch/epoch_type.h b/src/vt/epoch/epoch_type.h
index b5bd1c0e7..8c4792189 100644
--- a/src/vt/epoch/epoch_type.h
+++ b/src/vt/epoch/epoch_type.h
@@ -96,7 +96,9 @@ constexpr inline EpochType makeEpochZero() {
return EpochType{static_cast<EpochType::ImplType>(0ull)};
}
-inline auto format_as(EpochType e) { return *e; }
+inline auto format_as(EpochType e) {
+ return *e;
+}
}} /* end namespace vt::epoch */
diff --git a/src/vt/topos/index/dense/dense_array.h b/src/vt/topos/index/dense/dense_array.h
index 7df7faec0..5e9af0833 100644
--- a/src/vt/topos/index/dense/dense_array.h
+++ b/src/vt/topos/index/dense/dense_array.h
@@ -175,7 +175,7 @@ auto format_as(DenseIndexArray<T, nd> d) {
return d.toString();
}
-}} // end namespace vt::index
+}} // end namespace vt::index
#include "vt/topos/index/dense/dense_array.impl.h"
diff --git a/src/vt/vrt/collection/balance/temperedlb/tempered_msgs.h b/src/vt/vrt/collection/balance/temperedlb/tempered_msgs.h
index cf9682b93..eb1d23826 100644
--- a/src/vt/vrt/collection/balance/temperedlb/tempered_msgs.h
+++ b/src/vt/vrt/collection/balance/temperedlb/tempered_msgs.h
@@ -102,8 +102,8 @@ using RankSummaryType = std::tuple<BytesType, ClusterSummaryType>;
inline auto format_as(ClusterInfo const& e) {
auto fmt_str = "(load={},bytes={},intra=({},{})),home={},edge={}";
return fmt::format(
- fmt_str, e.load, e.bytes, e.intra_send_vol, e.intra_recv_vol,
- e.home_node, e.edge_weight
+ fmt_str, e.load, e.bytes, e.intra_send_vol, e.intra_recv_vol, e.home_node,
+ e.edge_weight
);
}
|
3ed309c to
0204739
Compare
TODO:
|
ec6d389 to
02c5cdc
Compare
| #if defined(__GNUC__) && !(defined(__clang__)) | ||
| #pragma GCC diagnostic push | ||
| #pragma GCC diagnostic ignored "-Wstringop-overflow" | ||
| #endif | ||
| while (begin != end) *out++ = static_cast<T>(*begin++); | ||
| #if defined(__GNUC__) && !(defined(__clang__)) | ||
| #pragma GCC diagnostic pop | ||
| #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.
Seems like something along the lines of https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94335. Let's just stick with #pragma GCC diagnostic ignored for this specific line 🤷
|
After disabling external fmt in cuda-11.2 build (02c5cdc) we are now facing another error: Note for later: we still need to make sure that we test external fmt configuration in some build. |
02c5cdc to
a15def4
Compare
1faad0d to
688d4a3
Compare
688d4a3 to
5f71f00
Compare
13a2066 to
63858da
Compare
33ee397 to
c9a8c9a
Compare
|
Cuda 11.2 currently fails with: This can be helped by setting Note: detecting This gets us a bit closer, but the build is still failing with: To be continued 🤷 Another thing to take care of: bump the required version of |
Upgrading Cuda to 11.4.3 solves the issue without any additional modifications. If we can afford this upgrade, I think this is the preferred way to handle this. |
|
We currently require fmt We need to pick a higher version, but it doesn't have to be |
cwschilly
left a comment
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.
Looks good, just a couple questions about includes
| #include <string> | ||
|
|
||
| #include INCLUDE_FMT_FORMAT |
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.
Is <string> included as part of fmt/format.h>?
Fixes #2397