Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Upgrade Boost to 1.84.0 #8679

Closed
wants to merge 1 commit into from
Closed

Upgrade Boost to 1.84.0 #8679

wants to merge 1 commit into from

Conversation

czentgr
Copy link
Collaborator

@czentgr czentgr commented Feb 6, 2024 •
edited
Loading

The boost version 1.84.0 fixes a bug that prevents
the addition of a group of inverse functions.
The fix was provided in Boost PR
boostorg/math#1007

The update occurs for the setup scripts for CentOS and Ubuntu. MacOS already installs the
latest available Boost version on brew.

In addition, the requirement for the Boost version is updated to make sure the proper version is used.
There is no known build problem when upgrading the version.
Fixes issue: #8624

@netlify Netlify
Copy link

netlify bot commented Feb 6, 2024 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit c697fbc
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/65e0b907d8186300086a46d0

@facebook-github-bot 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 Feb 6, 2024
@czentgr czentgr force-pushed the cz_upgrade_boost branch 2 times, most recently from 3113d4b to 91d5ecd Compare February 6, 2024 19:22
@czentgr czentgr requested a review from majetideepak February 6, 2024 22:30
@czentgr czentgr marked this pull request as ready for review February 6, 2024 22:31
@czentgr czentgr requested a review from kgpai February 7, 2024 22:37
@czentgr
Copy link
Collaborator Author

czentgr commented Feb 7, 2024

This PR also takes care of switching the source of boost away from artifactory boostorg.jfrog.io which is currently being also being proposed in PR #8211. However, it now gets the source from github using version tag instead.

@majetideepak
Copy link
Collaborator

majetideepak commented Feb 7, 2024 •
edited
Loading

Don't we have to mandate the minimum version of boost to be 1.84 in the CMakeLists.txt?
Can you also update boost minimum version in the README?
https://github.com/facebookincubator/velox/blob/main/CMake/resolve_dependency_modules/README.md

@czentgr
Copy link
Collaborator Author

czentgr commented Feb 7, 2024

Don't we have to mandate the minimum version of boost to be 1.84 in the CMakeLists.txt? Can you also update boost version in the README? https://github.com/facebookincubator/velox/blob/main/CMake/resolve_dependency_modules/README.md

I was thinking we could do this as a second step when the first dependent function is added but I suppose it can be done here as well.

Will update the README.

@majetideepak
Copy link
Collaborator

majetideepak commented Feb 8, 2024 •
edited
Loading

I was thinking we could do this as a second step when the first dependent function is added but I suppose it can be done here as well.

I see. Let's do this here so that the expectations are clear. Can you also add a link #5382 to the discussion on this issue in the description?

@majetideepak
Copy link
Collaborator

I guess we can re-open #5382 after this lands and the CI image is updated.

@majetideepak majetideepak changed the title Upgrade Boost to 1.84 Upgrade Boost to 1.84 and make it the minimum version Feb 8, 2024
@majetideepak
Copy link
Collaborator

majetideepak commented Feb 8, 2024 •
edited
Loading

@Yuhta, we are upgrading Boost to 1.84 and making it the minimum version to get the fix you made in the Boost library.
Do you have any thoughts on this? See PR #5382 for context.

@majetideepak majetideepak requested a review from Yuhta February 8, 2024 00:10
@majetideepak
Copy link
Collaborator

I see that some of the CI jobs require boost 1.84 to be installed in the CI image. We cannot mandate 1.84 in this PR unless we bundle Boost in those CI jobs.
Your original proposal to mandate 1.84 in a separate PR is cleaner. Let's do that. Sorry for the confusion.

@majetideepak majetideepak changed the title Upgrade Boost to 1.84 and make it the minimum version Upgrade Boost to 1.84 Feb 8, 2024
@kgpai
Copy link
Contributor

kgpai commented Feb 8, 2024

@majetideepak Will this require us to create a new Prestissimo container with boost 1.84 installed ?

@czentgr
Copy link
Collaborator Author

czentgr commented Feb 8, 2024 •
edited
Loading

@majetideepak Will this require us to create a new Prestissimo container with boost 1.84 installed ?

Yes, I think so. When the Velox is updated in prestissimo there should also be a new container as far as I understand it.
Edit: Maybe not. Boost is bundled. The prestissimo CI doesn't require the system to provide it which means at build time it would choose the bundled version. So maybe no new container is required.

@majetideepak
Copy link
Collaborator

@kgpai prestissimo will not need a new image until we mandate 1.84. Even then it will bundle if it does not find the right boost version.
@czentgr can we also revert the doc change until we mandate the version?

@kgpai
Copy link
Contributor

kgpai commented Feb 10, 2024

Thanks @majetideepak, One more question: folly is dependent on boost, if system folly is built against an older version and velox bundles a newer version , would that cause problems. Would we need to ask people to rerun setup scripts?

@majetideepak
Copy link
Collaborator

majetideepak commented Feb 12, 2024 •
edited
Loading

folly is dependent on boost, if system folly is built against an older version and velox bundles a newer version , would that cause problems

Technically, this is not a new problem. Even before this PR, someone could build folly with an older boost version and Velox vendors 1.81.0. Practically, there is a non-zero chance of conflict with 1.84.0.
But most developers are on MacOS and are using the latest boost (1.84.0) from brew. The recent dependency upgrade should have brought in 1.84.0 already.

@majetideepak majetideepak changed the title Upgrade Boost to 1.84 Upgrade Boost to 1.84.0 Feb 12, 2024
@czentgr czentgr force-pushed the cz_upgrade_boost branch 2 times, most recently from 6a08ca6 to d56ab1f Compare February 19, 2024 19:21
@czentgr
Copy link
Collaborator Author

czentgr commented Feb 19, 2024

@kgpai do you think we can move ahead with this?
Perhaps I could put in something in the PR description to re-run the boost and folly installs? Now that Deepak's PR got merged that allows individual package rebuilds in CentOS e.g. setup-centos.sh install_boost install_folly it should be fairly easy for users to update dependencies. Ubuntu already had this capability while MacOS in this case is handled using brew.

@kgpai
Copy link
Contributor

kgpai commented Feb 29, 2024

@czentgr can you make sure you rebase to latest main..

@czentgr
Upgrade Boost to 1.84
c697fbc 
The boost version 1.84 fixes a bug that prevents
the addition of a group of inverse functions.
The fix was provided in Boost PR
boostorg/math#1007

The boost_headers target was added to Boost first
in version 1.82 and are now removed from the
Velox bundled CMakeLists.

The update occurs for the setup scripts for CentOS and
Ubuntu. MacOS already installs the latest available
Boost version on brew.

In addition, the requirement for the Boost version
is updated to make sure the proper version is used.
There is no known build problem when upgrading the
version.
@facebook-github-bot
Copy link
Contributor

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

@mbasmanova
Copy link
Contributor

Should we update CMake/resolve_dependency_modules/README.md?

@kgpai
Copy link
Contributor

kgpai commented Feb 29, 2024

@mbasmanova Afaik, this change does not mandate 1.84 so we do not need to update the readme .
@czentgr, @majetideepak I understand that mandating this version will be a different PR that requires the bug fix provided in 1.84?

@czentgr
Copy link
Collaborator Author

czentgr commented Feb 29, 2024

Correct, we specifically removed the change to the README again (it was present before in the PR) because: #8679 (comment)

Now, the next step would be to add a function that makes use of the fix (one of the inverse functions).
Alternatively, we can make new PR to require the new version - update the README - and have the build images updated before adding a new function to see how it is going?

@kgpai
Copy link
Contributor

kgpai commented Feb 29, 2024

@czentgr If i am not mistaken, merging this change should build the images with boost 1.84 so the next pr can take it as supported (if not we will know by failures in said pr).

@czentgr
Copy link
Collaborator Author

czentgr commented Feb 29, 2024 •
edited
Loading

Then the next PR adding the function can then set this version as required (making sure the fix is present).

@facebook-github-bot
Copy link
Contributor

@kgpai merged this pull request in 6d50a1d.

@czentgr czentgr mentioned this pull request Mar 1, 2024
Closed
@zhli1142015
Copy link
Contributor

zhli1142015 commented Mar 2, 2024 •
edited
Loading

Looks CI failed with boost header file not found with latest main branch. Is it related to this change?
Thanks.
https://github.com/facebookincubator/velox/actions/runs/8119444457/job/22195340396?pr=8934

In file included from /home/runner/work/velox/velox/velox/velox/functions/sparksql/Register.cpp:40:
/home/runner/work/velox/velox/velox/./velox/functions/sparksql/String.h:20:10: fatal error: boost/locale.hpp: No such file or directory
   20 | #include <boost/locale.hpp>

@czentgr
Copy link
Collaborator Author

czentgr commented Mar 2, 2024

@zhli1142015 Yes. The benchmark builds the bundled boost version. Previously, it was installed from the package manager.
There are two options:

  1. The file is actually not needed.
  2. The dependency to boost has to be declared.

Looking into it. Trying to repro.

@czentgr
Copy link
Collaborator Author

czentgr commented Mar 2, 2024

@zhli1142015 I was able to locally repro and fix the issue: #8936

@PHILO-HE PHILO-HE mentioned this pull request Mar 15, 2024
Closed
@boneanxs boneanxs mentioned this pull request Mar 15, 2024
Merged
@codyschierbeck codyschierbeck mentioned this pull request Mar 26, 2024
Closed
facebook-github-bot pushed a commit that referenced this pull request Apr 12, 2024
@PHILO-HE @facebook-github-bot
Set minimum boost version to 1.77.0 (#9094)
7a36070 
Summary:
This [commit](6dba462) introduces some code to use a boost API: `boost::uuids::to_chars`, which is available since boost 1.77.0 (see [boost commit](boostorg/uuid@eaa4be7)). So I'm proposing to change the minimum version to 1.77.0.

Post related PR:
#8679

Pull Request resolved: #9094

Reviewed By: xiaoxmeng

Differential Revision: D56051595

Pulled By: kgpai

fbshipit-source-id: d1f5d78c45de203041ff865dad50d67abc7d84fb
Joe-Abraham pushed a commit to Joe-Abraham/velox that referenced this pull request Jun 7, 2024
@PHILO-HE @Joe-Abraham
Set minimum boost version to 1.77.0 (facebookincubator#9094)
60447dc 
Summary:
This [commit](facebookincubator@6dba462) introduces some code to use a boost API: `boost::uuids::to_chars`, which is available since boost 1.77.0 (see [boost commit](boostorg/uuid@eaa4be7)). So I'm proposing to change the minimum version to 1.77.0.

Post related PR:
facebookincubator#8679

Pull Request resolved: facebookincubator#9094

Reviewed By: xiaoxmeng

Differential Revision: D56051595

Pulled By: kgpai

fbshipit-source-id: d1f5d78c45de203041ff865dad50d67abc7d84fb
@czentgr czentgr deleted the cz_upgrade_boost branch July 31, 2024 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kgpai kgpai kgpai approved these changes

@Yuhta Yuhta Yuhta approved these changes

@majetideepak majetideepak majetideepak approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@czentgr @majetideepak @kgpai @facebook-github-bot @mbasmanova @zhli1142015 @Yuhta