Skip to content

[core] Upgrade abseil#48667

Merged
jjyao merged 1 commit intoray-project:masterfrom
dentiny:hjiang/upgrade-abseil
Nov 12, 2024
Merged

[core] Upgrade abseil#48667
jjyao merged 1 commit intoray-project:masterfrom
dentiny:hjiang/upgrade-abseil

Conversation

@dentiny
Copy link
Copy Markdown
Contributor

@dentiny dentiny commented Nov 9, 2024

I want to leverage new abseil utils, for example absl::NotNull, but it's not available in our current version.

The final goal is to keep all our third party dependencies to their latest version, but unfortunately,

  • The version in this PR is the latest which works without too much effort;
  • We need to upgrade rules_cc and protobuf to upgrade abseil further.

Signed-off-by: dentiny <dentinyhao@gmail.com>
@dentiny dentiny requested review from jjyao and rynewang November 9, 2024 04:42
# OpenCensus depends on Abseil so we have to explicitly pull it in.
# This is how diamond dependencies are prevented.
#
# TODO(owner): Upgrade abseil to latest version after protobuf updated, which requires to upgrade `rules_cc` first.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error when I was trying to further bump up version:

external/com_google_protobuf/src/google/protobuf/arena.cc:472:49: error: 'Layout' in namespace 'absl::lts_20240116::container_internal' does not name a template type
  472 |   using layout_type = absl::container_internal::Layout<
      |                                                 ^~~~~~
external/com_google_protobuf/src/google/protobuf/arena.cc:494:20: error: 'layout_type' does not name a type
  494 |   constexpr static layout_type Layout(size_t n) {
      |                    ^~~~~~~~~~~

which means our protobuf and abseil are too old so google side directly removes the interface :(

@dentiny dentiny added the go add ONLY when ready to merge, run all tests label Nov 9, 2024
@rynewang
Copy link
Copy Markdown
Contributor

rynewang commented Nov 9, 2024

what feature do you want from this abseil version? NonNull?

@rynewang
Copy link
Copy Markdown
Contributor

rynewang commented Nov 9, 2024

Just curious on what a absl::NonNull<T*> provides better than a T&?

@dentiny
Copy link
Copy Markdown
Contributor Author

dentiny commented Nov 9, 2024

what feature do you want from this abseil version? NonNull?

Yes, it's released in newer version.

Just curious on what a absl::NonNull<T*> provides better than a T&?

Old google coding style suggests using pointer instead of referencer for in-out params, I do see quite a few patterns in our codebase.
I think it's good to annotation null-ibility, via absl::Nullable or absl::Nonnull for better readability.

For your question, I think absl::NonNull<T*> has no difference than T&.

But overall, it's just a detailed example, which makes realized we're on a pretty stale abseil version;
I think for these key third-party dependencies we use on daily basis (at least for me), it's a good practice to keep them updated as possible.

@rynewang
Copy link
Copy Markdown
Contributor

rynewang commented Nov 9, 2024

hmm I am not convinced that 20230125.3 is a "pretty stale version" compared to 20230802.1.
Upgrading is not bad - but it may take a performance test or more so we typically want to do it when we have a real need whereas it seems absl::NonNull does not provide a lot of added value. To wisely use our eng time, I think e.g. cython version needs more care than abseil.

For this specific PR we can accept it because at least we can know it compiles by CI. Later if you want to upgrade something we can first have a talk.

@dentiny
Copy link
Copy Markdown
Contributor Author

dentiny commented Nov 9, 2024

hmm I am not convinced that 20230125.3 is a "pretty stale version" compared to 20230802.1.

As I mentioned in the PR description, I definitely want to upgrade the core libraries that we use to the latest version, but due the stale dependencies that we have (protobuf and rules cc), this is the latest workable release I could find.

@dentiny
Copy link
Copy Markdown
Contributor Author

dentiny commented Nov 9, 2024

Upgrading is not bad - but it may take a performance test or more so we typically want to do it when we have a real need

Well I don't agree with it TBH; I think dependencies should be checked and upgraded periodically.
If we keeping sticking to the old versions, it will be a nightmare for everyone if we find something really needed.

I get stuck into such situation in my previous company --- GCS SDK introduces key security features that we have to use, but our libraries is 5 major versions behind, which takes three engineers a full month to upgrade all necessary dependencies, I, as an eng which take part in the upgrade process, definitely don't want to have the same experience again :(

@dentiny
Copy link
Copy Markdown
Contributor Author

dentiny commented Nov 9, 2024

it seems absl::NonNull does not provide a lot of added value

Let's forget about the library, it's just a trigger which prompts me we're on a stale version.

@dentiny
Copy link
Copy Markdown
Contributor Author

dentiny commented Nov 9, 2024

To wisely use our eng time, I think e.g. cython version needs more care than abseil.

Later if you want to upgrade something we can first have a talk.

I think there're two things here:

  • I agree certain libraries have higher priority than others, meanwhile certain libraries are also easier to upgrade than others;
  • I generally follow the "volunteer rule", which means
    • I volunteer to upgrade core libraries, even if it's not our team's mandatory responsibility
    • For eng time, I'm OK with spending weekends and after-works hours; for reviewers, I expect the code change to be minimal; for performance and functionality affect, I fully trust our CI
    • Having a discussion within anyone before upgrading a lib looks weird to me...

@rynewang
Copy link
Copy Markdown
Contributor

rynewang commented Nov 9, 2024

volunteer mode also sounds ok to me. We need to take a more closed look at each release's performance impacts though.

@dentiny
Copy link
Copy Markdown
Contributor Author

dentiny commented Nov 11, 2024

There're some auto-upgrade bots: https://github.com/apps/renovate
But doesn't seem to support C++ third-party libraries.

@jjyao jjyao merged commit 84092c1 into ray-project:master Nov 12, 2024
JP-sDEV pushed a commit to JP-sDEV/ray that referenced this pull request Nov 14, 2024
Signed-off-by: dentiny <dentinyhao@gmail.com>
rynewang pushed a commit that referenced this pull request Dec 2, 2024
… using modern absl (#48722)

In
[src/ray/object_manager/common.cc](https://github.com/ray-project/ray/blob/e393a716d8742a987a36df555defb2ca90bb94d4/src/ray/object_manager/common.cc)
we extensively use `absl::StrCat` without explicitely importing the
associated header `absl/strings/str_cat.h`. It worked fine previously,
as `absl/strings/str_format.h` transitively included that via absl
internal headers. However, somewhere in 2023-2024, absl has been cleaned
up from unintentional transitive includes, breaking `ray` builds against
modern absl. This PR fixes that.

## Effect on backward/forward compatability
None. Builds against older absl versions will continue to work as
before, as `absl::StrCat` was always declared in `str_cat.h` and
including that was always the intended way to import this symbol.

## Related issues/PRs:
+ #48667

---------

Signed-off-by: gorloffslava <31761951+gorloffslava@users.noreply.github.com>
Signed-off-by: Slava Gorlov <31761951+gorloffslava@users.noreply.github.com>
jecsand838 pushed a commit to jecsand838/ray that referenced this pull request Dec 4, 2024
… using modern absl (ray-project#48722)

In
[src/ray/object_manager/common.cc](https://github.com/ray-project/ray/blob/e393a716d8742a987a36df555defb2ca90bb94d4/src/ray/object_manager/common.cc)
we extensively use `absl::StrCat` without explicitely importing the
associated header `absl/strings/str_cat.h`. It worked fine previously,
as `absl/strings/str_format.h` transitively included that via absl
internal headers. However, somewhere in 2023-2024, absl has been cleaned
up from unintentional transitive includes, breaking `ray` builds against
modern absl. This PR fixes that.

## Effect on backward/forward compatability
None. Builds against older absl versions will continue to work as
before, as `absl::StrCat` was always declared in `str_cat.h` and
including that was always the intended way to import this symbol.

## Related issues/PRs:
+ ray-project#48667

---------

Signed-off-by: gorloffslava <31761951+gorloffslava@users.noreply.github.com>
Signed-off-by: Slava Gorlov <31761951+gorloffslava@users.noreply.github.com>
Signed-off-by: Connor Sanders <connor@elastiflow.com>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Dec 17, 2024
… using modern absl (ray-project#48722)

In
[src/ray/object_manager/common.cc](https://github.com/ray-project/ray/blob/e393a716d8742a987a36df555defb2ca90bb94d4/src/ray/object_manager/common.cc)
we extensively use `absl::StrCat` without explicitely importing the
associated header `absl/strings/str_cat.h`. It worked fine previously,
as `absl/strings/str_format.h` transitively included that via absl
internal headers. However, somewhere in 2023-2024, absl has been cleaned
up from unintentional transitive includes, breaking `ray` builds against
modern absl. This PR fixes that.

## Effect on backward/forward compatability
None. Builds against older absl versions will continue to work as
before, as `absl::StrCat` was always declared in `str_cat.h` and
including that was always the intended way to import this symbol.

## Related issues/PRs:
+ ray-project#48667

---------

Signed-off-by: gorloffslava <31761951+gorloffslava@users.noreply.github.com>
Signed-off-by: Slava Gorlov <31761951+gorloffslava@users.noreply.github.com>
Signed-off-by: ujjawal-khare <ujjawal.khare@dream11.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-backlog go add ONLY when ready to merge, run all tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants