Upgrade fmt to 10.1.1 (from 8.0.1) #7941

assignUser · 2023-12-08T18:31:38Z

fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during #7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1).

Closes #7896

netlify · 2023-12-08T18:31:43Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`430e425`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/65810dfb52f558000865d90d

assignUser · 2023-12-08T18:32:09Z

cc @majetideepak

kgpai · 2023-12-08T23:16:50Z

LG, Any reason its in draft ?

assignUser · 2023-12-08T23:19:35Z

Yes we need to fix these issues first: https://app.circleci.com/pipelines/github/facebookincubator/velox/40342/workflows/1e588585-b6ce-4c90-b9f7-48cfd92a4dba/jobs/275478/parallel-runs/0/steps/0-106?invite=true#step-106-326445_26

See also the linked issue for more info!

majetideepak

@assignUser Nice progress! Some high-level comments. Can you apply them to other changes as well? Thanks.

velox/common/caching/AsyncDataCache.cpp

velox/common/memory/ByteStream.cpp

velox/common/memory/tests/MemoryAllocatorTest.cpp

velox/common/memory/tests/MemoryManagerTest.cpp

velox/common/memory/tests/MemoryPoolTest.cpp

velox/common/memory/tests/MockSharedArbitratorTest.cpp

velox/connectors/fuzzer/FuzzerConnectorSplit.h

velox/core/PlanNode.h

velox/functions/prestosql/Arithmetic.h

velox/exec/Operator.h

velox/common/base/FmtStdFormatters.h

assignUser · 2023-12-13T21:49:21Z

Looks like everything builds correctly now but we have some fuzzer and unit test failures.

majetideepak · 2023-12-14T11:42:37Z

@assignUser, Do you see these unit test failures in your local build as well? Or are these only on CI?

assignUser · 2023-12-14T12:01:54Z

@majetideepak testing that now, it's surprisingly involved to run the failing test from CI locally... maybe velox_exec_test should be split up to not be ~1k tests that take 10 minutes to run^^

assignUser · 2023-12-14T12:04:17Z

Also is it intentional that ✔️ tests produce pages worth of errors? (which is also probably the reason why the log is so large that CCI doesn't save the full log)

E1214 13:03:06.053968 879571 Exceptions.h:69] Line: /home/jwj/code/velox/./velox/exec/Spiller.h:181, Function:setPartitionsSpilled, Expression: type_ == Spiller::Type::kHashJoinProbe (0 vs. 3) Unexpected spiller type: , Source: RUNTIME, ErrorCode: INVALID_STATE
E1214 13:03:06.054023 879571 Exceptions.h:69] Line: /home/jwj/code/velox/velox/exec/Spiller.cpp:484, Function:spill, Expression: !finalized_ Spiller has been finalized, Source: RUNTIME, ErrorCode: INVALID_STATE
E1214 13:03:06.054060 879571 Exceptions.h:69] Line: /home/jwj/code/velox/velox/exec/Spiller.cpp:521, Function:spill, Expression: !finalized_ Spiller has been finalized, Source: RUNTIME, ErrorCode: INVALID_STATE
E1214 13:03:06.054097 879571 Exceptions.h:69] Line: /home/jwj/code/velox/velox/exec/Spiller.cpp:479, Function:spill, Expression: type_ == Type::kAggregateOutput (0 vs. 1), Source: RUNTIME, ErrorCode: INVALID_STATE
[       OK ] SpillerTest/NoHashJoin.spilFew/1 (481 ms)

majetideepak · 2023-12-14T12:08:21Z

Also is it intentional that ✔️ tests produce pages worth of errors?

I noticed this as well. I don't think this is the intention. We should fix this.

majetideepak · 2023-12-14T12:10:47Z

it's surprisingly involved to run the failing test from CI locally.

You should be able to use --gtest_filter to pick a particular test in the target. Can you try this test that seems to fail in the linux-adapters CI?
./cmake-build-debug/velox/core/tests/velox_core_test --gtest_filter=TypedExprSerDeTest.concat

assignUser · 2023-12-14T12:13:31Z

ah I was relying on the ctest -R option that only filters by test executable name. Good to know. The test passes.

assignUser · 2023-12-14T15:53:59Z

It seems like fbthrift (in the version we install) is not compatible with newer fmt https://github.com/facebookincubator/velox/actions/runs/7210627614/job/19644209080?pr=7941#step:5:5004

assignUser · 2023-12-14T16:56:17Z

I am pretty sure that the ci issues are mostly caused by version conflicts with the pre-built images. I am going to rebuild the image and upload under a different tag (combined with #7700 ) and use that temporarily maybe not...

kgpai · 2024-01-05T19:21:33Z

Starting import of this to check the changes required internally. Thanks for this change @assignUser !

facebook-github-bot · 2024-01-05T19:22:01Z

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

kgpai · 2024-01-10T19:11:10Z

@assignUser @majetideepak Just wanted to update you folks: There are some significant number of internal build failures due to this . I will need a few days to resolve them.

assignUser · 2024-01-11T03:33:10Z

There is also the issue with the test @majetideepak mentioned, it seems this is for escaping postgres? Good you shed some light on that and fix the test/the formating for that bit as I don't understand what's intended there. Apart from that this should be fine (on this side at least^^).

Summary: fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1). Closes facebookincubator#7896 Pull Request resolved: facebookincubator#7941 Differential Revision: D52880145 Pulled By: kgpai fbshipit-source-id: f11b62cb69aee0776170e972948321410cfd4802

Summary: Pull Request resolved: facebookincubator#8434 fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1). Closes facebookincubator#7896 Pull Request resolved: facebookincubator#7941 Differential Revision: D52880145 Pulled By: kgpai fbshipit-source-id: ae9d58575d416519d8d0447380564083da88b4a3

Summary: Pull Request resolved: facebookincubator#8434 fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1). Closes facebookincubator#7896 Pull Request resolved: facebookincubator#7941 Differential Revision: D52880145 Pulled By: kgpai fbshipit-source-id: a3ff1859a0df00a9b49f400665f0b16d6f27acba

Summary: Pull Request resolved: facebookincubator#8434 fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1). Closes facebookincubator#7896 Pull Request resolved: facebookincubator#7941 Differential Revision: D52880145 Pulled By: kgpai fbshipit-source-id: ea6797059930351aad170eee19026b183d352fd2

Summary: Pull Request resolved: facebookincubator#8434 fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1). Closes facebookincubator#7896 Pull Request resolved: facebookincubator#7941 Reviewed By: Yuhta Differential Revision: D52880145 Pulled By: kgpai fbshipit-source-id: b0724355657f2303d115da5b82572f1cbf3ada72

Summary: Pull Request resolved: facebookincubator#8434 fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1). Closes facebookincubator#7896 Pull Request resolved: facebookincubator#7941 Reviewed By: Yuhta Differential Revision: D52880145 Pulled By: kgpai fbshipit-source-id: 773c88c41c4b7c786fbf108e4257cfe7c3cc2594

Summary: Pull Request resolved: facebookincubator#8434 fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1). Closes facebookincubator#7896 Pull Request resolved: facebookincubator#7941 Reviewed By: Yuhta Differential Revision: D52880145 Pulled By: kgpai fbshipit-source-id: 68db12f6c67e968562122d324d0a9f9e06ea1f62

kgpai · 2024-01-19T20:57:54Z

velox/core/Expressions.h

@@ -320,11 +320,10 @@ class FieldAccessTypedExpr : public ITypedExpr {

  std::string toString() const override {
    if (inputs().empty()) {
-      return fmt::format("{}", std::quoted(name(), '"', '"'));
+      return fmt::format("{:?}", name());


FYI, This change breaks koski internally, so not sure why this change is being made. I am removing this in #8434 (fyi I wont merge 8434 but will merge this one after I get passing signals from 8434. I will also be redoing those changes to this PR).

kgpai · 2024-01-19T20:59:30Z

velox/benchmarks/basic/LikeTpchBenchmark.cpp

@@ -240,3 +240,10 @@ int main(int argc, char* argv[]) {

  return 0;
 }
+
+template <>
+struct fmt::formatter<TpchBenchmarkCase> : formatter<int> {


FYI, All the formatter<...> can be replaced with the much simpler format_as() . I will make those changes too..

that will make it not be backwards compatible with < 9

(just an fyi as prestissimo uses fmt 8.1 afaik)

Ah ok - This was failing in buck with specialization before instantiation , but I think theres another way around it.

kgpai · 2024-01-19T21:00:15Z

scripts/setup-circleci.sh

+wget_and_untar https://archive.apache.org/dist/hadoop/common/hadoop-2.10.1/hadoop-2.10.1.tar.gz hadoop 
+wget_and_untar https://github.com/protocolbuffers/protobuf/releases/download/v21.4/protobuf-all-21.4.tar.gz protobuf 
+
+FB_OS_VERSION="v2023.12.04.00"


Is this intentional do we plan to upgrade folly and format in this pr ?

Kind of but not really, only the preinstalled version in the container. This is necessary as mvfast doen't have a version for our old tag and is required for fbthrift. I tried a few older tags of mvfast but I was unable to find a combination of tags of folly and the others that compiled successfully. That's also why I sed out the deprecated annotation from the header further down in the script.

Summary: Pull Request resolved: facebookincubator#8434 fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1). Closes facebookincubator#7896 Pull Request resolved: facebookincubator#7941 Reviewed By: Yuhta Differential Revision: D52880145 Pulled By: kgpai fbshipit-source-id: 399ff634da95321a05b342adb2ead9f14792ed5c

Summary: Pull Request resolved: facebookincubator#8434 fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1). Closes facebookincubator#7896 Pull Request resolved: facebookincubator#7941 Reviewed By: Yuhta Differential Revision: D52880145 Pulled By: kgpai fbshipit-source-id: 0c4d1ce4f18eca10f69cea7301cc849680767b95

Summary: Pull Request resolved: facebookincubator#8434 fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1). Closes facebookincubator#7896 Pull Request resolved: facebookincubator#7941 Reviewed By: Yuhta Differential Revision: D52880145 Pulled By: kgpai fbshipit-source-id: 1d8704484a14b9f03ee8c709ed283d5e586d35c4

Summary: Pull Request resolved: facebookincubator#8434 fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1). Closes facebookincubator#7896 Pull Request resolved: facebookincubator#7941 Reviewed By: Yuhta Differential Revision: D52880145 Pulled By: kgpai fbshipit-source-id: 1a1c070399a33d0202b284f045db3ab5081966cb

Summary: Pull Request resolved: facebookincubator#8434 fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1). Closes facebookincubator#7896 Pull Request resolved: facebookincubator#7941 Reviewed By: Yuhta Differential Revision: D52880145 Pulled By: kgpai fbshipit-source-id: e847ab3bb268d950a1d82ad2f6b3cd9d18baa70d

Summary: Pull Request resolved: facebookincubator#8434 fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1). Closes facebookincubator#7896 Pull Request resolved: facebookincubator#7941 Reviewed By: Yuhta Differential Revision: D52880145 Pulled By: kgpai fbshipit-source-id: f49db1561006fdd5c876b28a727f104d5aab4a58

Summary: Pull Request resolved: facebookincubator#8434 fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1). Closes facebookincubator#7896 Pull Request resolved: facebookincubator#7941 Reviewed By: Yuhta Differential Revision: D52880145 Pulled By: kgpai fbshipit-source-id: 4c8aa99b6be6a7dc1d2f4854145830f1254fbd2c

Summary: Pull Request resolved: facebookincubator#8434 fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1). Closes facebookincubator#7896 Pull Request resolved: facebookincubator#7941 Reviewed By: Yuhta Differential Revision: D52880145 Pulled By: kgpai fbshipit-source-id: cf96653f76ff2d3c8c802c8230107515a36cb440

Summary: Pull Request resolved: facebookincubator#8434 fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1). Closes facebookincubator#7896 Pull Request resolved: facebookincubator#7941 Reviewed By: Yuhta Differential Revision: D52880145 Pulled By: kgpai fbshipit-source-id: 8f7e8d8d69ac3260a040eb8ae0b43432c87f6841

facebook-github-bot · 2024-01-24T02:19:52Z

@kgpai merged this pull request in 1deb036.

conbench-facebook · 2024-01-24T03:00:14Z

Conbench analyzed the 1 benchmark run on commit 1deb0368.

There weren't enough matching historic benchmark results to make a call on whether there were regressions.

The full Conbench report has more details.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 8, 2023

assignUser changed the title ~~[wip] Upgrade fmt to 10.1.1 (from 8.0.1)~~ Upgrade fmt to 10.1.1 (from 8.0.1) Dec 8, 2023

assignUser marked this pull request as draft December 8, 2023 18:31

kgpai approved these changes Dec 8, 2023

View reviewed changes

kgpai mentioned this pull request Dec 8, 2023

Upgrade folly to v2023.12.04.00 (from v2022.11.14.00) #7700

Closed

assignUser force-pushed the bump-fmt branch from c5db05b to afdbae1 Compare December 13, 2023 02:30

majetideepak reviewed Dec 13, 2023

View reviewed changes

velox/core/PlanNode.h Show resolved Hide resolved

andrewseidl reviewed Dec 13, 2023

View reviewed changes

velox/functions/prestosql/Arithmetic.h Outdated Show resolved Hide resolved

andrewseidl reviewed Dec 13, 2023

View reviewed changes

velox/exec/Operator.h Show resolved Hide resolved

assignUser commented Dec 13, 2023

View reviewed changes

velox/common/base/FmtStdFormatters.h Show resolved Hide resolved

assignUser added 6 commits December 15, 2023 01:17

bump fmt to 10.1.1

d773004

add formatters and other fixes for fmt 8.1+

78ff5c3

more changes

ca6d5a9

format

6f8976d

add suggestions from review

2509d18

additional changes

8dc32b2

kgpai mentioned this pull request Jan 10, 2024

M1 build failing with C++ compilation errors #8331

Open

kgpai reviewed Jan 19, 2024

View reviewed changes

Yuhta approved these changes Jan 23, 2024

View reviewed changes

facebook-github-bot closed this in 1deb036 Jan 24, 2024

facebook-github-bot added the Merged label Jan 24, 2024

assignUser mentioned this pull request Jan 24, 2024

Do Not Merge : Upgrade fmt to 10.1.1 (from 8.0.1) (#7941) #8434

Closed

assignUser deleted the bump-fmt branch January 25, 2024 00:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade fmt to 10.1.1 (from 8.0.1) #7941

Upgrade fmt to 10.1.1 (from 8.0.1) #7941

assignUser commented Dec 8, 2023 •

edited

Loading

netlify bot commented Dec 8, 2023 •

edited

Loading

assignUser commented Dec 8, 2023

kgpai commented Dec 8, 2023

assignUser commented Dec 8, 2023

majetideepak left a comment

assignUser commented Dec 13, 2023

majetideepak commented Dec 14, 2023

assignUser commented Dec 14, 2023

assignUser commented Dec 14, 2023 •

edited

Loading

majetideepak commented Dec 14, 2023

majetideepak commented Dec 14, 2023

assignUser commented Dec 14, 2023

assignUser commented Dec 14, 2023

assignUser commented Dec 14, 2023 •

edited

Loading

kgpai commented Jan 5, 2024

facebook-github-bot commented Jan 5, 2024

kgpai commented Jan 10, 2024

assignUser commented Jan 11, 2024

kgpai Jan 19, 2024

kgpai Jan 19, 2024

assignUser Jan 19, 2024

assignUser Jan 19, 2024 •

edited

Loading

kgpai Jan 19, 2024

kgpai Jan 19, 2024

assignUser Jan 20, 2024

facebook-github-bot commented Jan 24, 2024

conbench-facebook bot commented Jan 24, 2024

Upgrade fmt to 10.1.1 (from 8.0.1) #7941

Upgrade fmt to 10.1.1 (from 8.0.1) #7941

Conversation

assignUser commented Dec 8, 2023 • edited Loading

netlify bot commented Dec 8, 2023 • edited Loading

✅ Deploy Preview for meta-velox canceled.

assignUser commented Dec 8, 2023

kgpai commented Dec 8, 2023

assignUser commented Dec 8, 2023

majetideepak left a comment

Choose a reason for hiding this comment

assignUser commented Dec 13, 2023

majetideepak commented Dec 14, 2023

assignUser commented Dec 14, 2023

assignUser commented Dec 14, 2023 • edited Loading

majetideepak commented Dec 14, 2023

majetideepak commented Dec 14, 2023

assignUser commented Dec 14, 2023

assignUser commented Dec 14, 2023

assignUser commented Dec 14, 2023 • edited Loading

kgpai commented Jan 5, 2024

facebook-github-bot commented Jan 5, 2024

kgpai commented Jan 10, 2024

assignUser commented Jan 11, 2024

kgpai Jan 19, 2024

Choose a reason for hiding this comment

kgpai Jan 19, 2024

Choose a reason for hiding this comment

assignUser Jan 19, 2024

Choose a reason for hiding this comment

assignUser Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

kgpai Jan 19, 2024

Choose a reason for hiding this comment

kgpai Jan 19, 2024

Choose a reason for hiding this comment

assignUser Jan 20, 2024

Choose a reason for hiding this comment

facebook-github-bot commented Jan 24, 2024

conbench-facebook bot commented Jan 24, 2024

assignUser commented Dec 8, 2023 •

edited

Loading

netlify bot commented Dec 8, 2023 •

edited

Loading

assignUser commented Dec 14, 2023 •

edited

Loading

assignUser commented Dec 14, 2023 •

edited

Loading

assignUser Jan 19, 2024 •

edited

Loading