[core] Cleanup metrics stuff + fix gcc >10 build by dayshah · Pull Request #56514 · ray-project/ray

dayshah · 2025-09-14T06:46:30Z

Why are these changes needed?

Fixing the build for my gcc devbox, was struggling to disambiguate between map with string_view vs. map with string. Changed Record to always take string_view. The string overload is only used in Cython so just have a separate RecordForCython that's not part of the MetricInterface. We can't use string_view in Cython because our Cython version isn't new enough.

Also killing tag_defs.cc, it just had one line tag registrations that could go in the headers. There were some tags that were unused so killed them.
There were also some string constants that could just be put in the one spot they were used so the files that use it don't need the dependency.

The biggest change is changing the record to take a vector of pairs instead of an unordered map. We didn't use the map aspect of it, so a vector is more efficient.

Record was also taking by const ref before and then trying to move, so the move wasn't actually happening. Taking by value now so that it actually moves.

Signed-off-by: dayshah <dhyey2019@gmail.com>

edoakes · 2025-09-15T17:50:17Z

@can-anyscale PTAL

can-anyscale

lgtm; just a question on the c++ apis

can-anyscale · 2025-09-15T22:19:28Z

cpp/include/ray/api/metric.h

  /// \param[in] value The value that we record.
  /// \param[in] tags The map tag values that we want to record
-  void Observe(double value, const std::unordered_map<std::string, std::string> &Tags);
+  void Observe(double value,


oh i actually wasn't aware of these APIs; are they our internal API or user facing APIs? If the later can we actually change the signatures?

Ya I was wondering this too, it looks user facing @jjyao on if we can change?

I think these are technically public APIs

kk, I guess we can remove the changes of cpp/include in this PR, or communicate the deprecation plan publicly etc. then it is good to merge

updated to just revert the cpp api back, just turning into a string vector at the end before calling into core. We're killing a copy on the core side so even though we're adding a cheaper copy (bc string_views), it's still an improvement.

sg that's what I would've suggested

can-anyscale · 2025-09-15T22:23:48Z

src/ray/stats/metric.h

         const std::vector<std::string> &tag_keys = {});

-  virtual ~Metric();
+  ~Metric() override;


qq, that is the effect of this change?

No functional change but only the base class needs the destructor to be virtual and the base class is MetricInterface now, everything else is overriding

Signed-off-by: dayshah <dhyey2019@gmail.com>

This reverts commit 2330b62.

## Why are these changes needed? Fixing the build for my gcc devbox, was struggling to disambiguate between map with string_view vs. map with string. Changed Record to always take string_view. The string overload is only used in Cython so just have a separate RecordForCython that's not part of the MetricInterface. We can't use string_view in Cython because our Cython version isn't new enough. Also killing tag_defs.cc, it just had one line tag registrations that could go in the headers. There were some tags that were unused so killed them. There were also some string constants that could just be put in the one spot they were used so the files that use it don't need the dependency. The biggest change is changing the record to take a vector of pairs instead of an unordered map. We didn't use the map aspect of it, so a vector is more efficient. Record was also taking by const ref before and then trying to move, so the move wasn't actually happening. Taking by value now so that it actually moves. --------- Signed-off-by: dayshah <dhyey2019@gmail.com> Signed-off-by: Zhiqiang Ma <zhiqiang.ma@intel.com>

## Why are these changes needed? Fixing the build for my gcc devbox, was struggling to disambiguate between map with string_view vs. map with string. Changed Record to always take string_view. The string overload is only used in Cython so just have a separate RecordForCython that's not part of the MetricInterface. We can't use string_view in Cython because our Cython version isn't new enough. Also killing tag_defs.cc, it just had one line tag registrations that could go in the headers. There were some tags that were unused so killed them. There were also some string constants that could just be put in the one spot they were used so the files that use it don't need the dependency. The biggest change is changing the record to take a vector of pairs instead of an unordered map. We didn't use the map aspect of it, so a vector is more efficient. Record was also taking by const ref before and then trying to move, so the move wasn't actually happening. Taking by value now so that it actually moves. --------- Signed-off-by: dayshah <dhyey2019@gmail.com> Signed-off-by: zac <zac@anyscale.com>

## Why are these changes needed? Fixing the build for my gcc devbox, was struggling to disambiguate between map with string_view vs. map with string. Changed Record to always take string_view. The string overload is only used in Cython so just have a separate RecordForCython that's not part of the MetricInterface. We can't use string_view in Cython because our Cython version isn't new enough. Also killing tag_defs.cc, it just had one line tag registrations that could go in the headers. There were some tags that were unused so killed them. There were also some string constants that could just be put in the one spot they were used so the files that use it don't need the dependency. The biggest change is changing the record to take a vector of pairs instead of an unordered map. We didn't use the map aspect of it, so a vector is more efficient. Record was also taking by const ref before and then trying to move, so the move wasn't actually happening. Taking by value now so that it actually moves. --------- Signed-off-by: dayshah <dhyey2019@gmail.com> Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>

## Why are these changes needed? Fixing the build for my gcc devbox, was struggling to disambiguate between map with string_view vs. map with string. Changed Record to always take string_view. The string overload is only used in Cython so just have a separate RecordForCython that's not part of the MetricInterface. We can't use string_view in Cython because our Cython version isn't new enough. Also killing tag_defs.cc, it just had one line tag registrations that could go in the headers. There were some tags that were unused so killed them. There were also some string constants that could just be put in the one spot they were used so the files that use it don't need the dependency. The biggest change is changing the record to take a vector of pairs instead of an unordered map. We didn't use the map aspect of it, so a vector is more efficient. Record was also taking by const ref before and then trying to move, so the move wasn't actually happening. Taking by value now so that it actually moves. --------- Signed-off-by: dayshah <dhyey2019@gmail.com> Signed-off-by: Marco Stephan <marco@magic.dev>

Signed-off-by: dayshah <dhyey2019@gmail.com>

## Why are these changes needed? Fixing the build for my gcc devbox, was struggling to disambiguate between map with string_view vs. map with string. Changed Record to always take string_view. The string overload is only used in Cython so just have a separate RecordForCython that's not part of the MetricInterface. We can't use string_view in Cython because our Cython version isn't new enough. Also killing tag_defs.cc, it just had one line tag registrations that could go in the headers. There were some tags that were unused so killed them. There were also some string constants that could just be put in the one spot they were used so the files that use it don't need the dependency. The biggest change is changing the record to take a vector of pairs instead of an unordered map. We didn't use the map aspect of it, so a vector is more efficient. Record was also taking by const ref before and then trying to move, so the move wasn't actually happening. Taking by value now so that it actually moves. --------- Signed-off-by: dayshah <dhyey2019@gmail.com> Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>

https://buildkite.com/ray-project/postmerge-macos/builds/8257#01997de0-1e13-4954-a370-6255f650ca17 mac c++ / java tests broke after #56514. Fixing by reverting to have tag_defs.cc define the vars in a separate file and not just in the header. Why does this fix it - I have no idea... for some reason the c++ api dynamic linking madness doesn't like the inlined vars on the mac build specifically??? --------- Signed-off-by: dayshah <dhyey2019@gmail.com>

## Why are these changes needed? Fixing the build for my gcc devbox, was struggling to disambiguate between map with string_view vs. map with string. Changed Record to always take string_view. The string overload is only used in Cython so just have a separate RecordForCython that's not part of the MetricInterface. We can't use string_view in Cython because our Cython version isn't new enough. Also killing tag_defs.cc, it just had one line tag registrations that could go in the headers. There were some tags that were unused so killed them. There were also some string constants that could just be put in the one spot they were used so the files that use it don't need the dependency. The biggest change is changing the record to take a vector of pairs instead of an unordered map. We didn't use the map aspect of it, so a vector is more efficient. Record was also taking by const ref before and then trying to move, so the move wasn't actually happening. Taking by value now so that it actually moves. --------- Signed-off-by: dayshah <dhyey2019@gmail.com> Signed-off-by: Douglas Strodtman <douglas@anyscale.com>

https://buildkite.com/ray-project/postmerge-macos/builds/8257#01997de0-1e13-4954-a370-6255f650ca17 mac c++ / java tests broke after #56514. Fixing by reverting to have tag_defs.cc define the vars in a separate file and not just in the header. Why does this fix it - I have no idea... for some reason the c++ api dynamic linking madness doesn't like the inlined vars on the mac build specifically??? --------- Signed-off-by: dayshah <dhyey2019@gmail.com> Signed-off-by: Douglas Strodtman <douglas@anyscale.com>

…6915) https://buildkite.com/ray-project/postmerge-macos/builds/8257#01997de0-1e13-4954-a370-6255f650ca17 mac c++ / java tests broke after ray-project#56514. Fixing by reverting to have tag_defs.cc define the vars in a separate file and not just in the header. Why does this fix it - I have no idea... for some reason the c++ api dynamic linking madness doesn't like the inlined vars on the mac build specifically??? --------- Signed-off-by: dayshah <dhyey2019@gmail.com> Signed-off-by: Josh Kodi <joshkodi@gmail.com>

## Why are these changes needed? Fixing the build for my gcc devbox, was struggling to disambiguate between map with string_view vs. map with string. Changed Record to always take string_view. The string overload is only used in Cython so just have a separate RecordForCython that's not part of the MetricInterface. We can't use string_view in Cython because our Cython version isn't new enough. Also killing tag_defs.cc, it just had one line tag registrations that could go in the headers. There were some tags that were unused so killed them. There were also some string constants that could just be put in the one spot they were used so the files that use it don't need the dependency. The biggest change is changing the record to take a vector of pairs instead of an unordered map. We didn't use the map aspect of it, so a vector is more efficient. Record was also taking by const ref before and then trying to move, so the move wasn't actually happening. Taking by value now so that it actually moves. --------- Signed-off-by: dayshah <dhyey2019@gmail.com>

…6915) https://buildkite.com/ray-project/postmerge-macos/builds/8257#01997de0-1e13-4954-a370-6255f650ca17 mac c++ / java tests broke after ray-project#56514. Fixing by reverting to have tag_defs.cc define the vars in a separate file and not just in the header. Why does this fix it - I have no idea... for some reason the c++ api dynamic linking madness doesn't like the inlined vars on the mac build specifically??? --------- Signed-off-by: dayshah <dhyey2019@gmail.com>

## Why are these changes needed? Fixing the build for my gcc devbox, was struggling to disambiguate between map with string_view vs. map with string. Changed Record to always take string_view. The string overload is only used in Cython so just have a separate RecordForCython that's not part of the MetricInterface. We can't use string_view in Cython because our Cython version isn't new enough. Also killing tag_defs.cc, it just had one line tag registrations that could go in the headers. There were some tags that were unused so killed them. There were also some string constants that could just be put in the one spot they were used so the files that use it don't need the dependency. The biggest change is changing the record to take a vector of pairs instead of an unordered map. We didn't use the map aspect of it, so a vector is more efficient. Record was also taking by const ref before and then trying to move, so the move wasn't actually happening. Taking by value now so that it actually moves. --------- Signed-off-by: dayshah <dhyey2019@gmail.com>

…6915) https://buildkite.com/ray-project/postmerge-macos/builds/8257#01997de0-1e13-4954-a370-6255f650ca17 mac c++ / java tests broke after ray-project#56514. Fixing by reverting to have tag_defs.cc define the vars in a separate file and not just in the header. Why does this fix it - I have no idea... for some reason the c++ api dynamic linking madness doesn't like the inlined vars on the mac build specifically??? --------- Signed-off-by: dayshah <dhyey2019@gmail.com>

…6915) https://buildkite.com/ray-project/postmerge-macos/builds/8257#01997de0-1e13-4954-a370-6255f650ca17 mac c++ / java tests broke after ray-project#56514. Fixing by reverting to have tag_defs.cc define the vars in a separate file and not just in the header. Why does this fix it - I have no idea... for some reason the c++ api dynamic linking madness doesn't like the inlined vars on the mac build specifically??? --------- Signed-off-by: dayshah <dhyey2019@gmail.com> Signed-off-by: Aydin Abiar <aydin@anyscale.com>

## Why are these changes needed? Fixing the build for my gcc devbox, was struggling to disambiguate between map with string_view vs. map with string. Changed Record to always take string_view. The string overload is only used in Cython so just have a separate RecordForCython that's not part of the MetricInterface. We can't use string_view in Cython because our Cython version isn't new enough. Also killing tag_defs.cc, it just had one line tag registrations that could go in the headers. There were some tags that were unused so killed them. There were also some string constants that could just be put in the one spot they were used so the files that use it don't need the dependency. The biggest change is changing the record to take a vector of pairs instead of an unordered map. We didn't use the map aspect of it, so a vector is more efficient. Record was also taking by const ref before and then trying to move, so the move wasn't actually happening. Taking by value now so that it actually moves. --------- Signed-off-by: dayshah <dhyey2019@gmail.com> Signed-off-by: Future-Outlier <eric901201@gmail.com>

…6915) https://buildkite.com/ray-project/postmerge-macos/builds/8257#01997de0-1e13-4954-a370-6255f650ca17 mac c++ / java tests broke after ray-project#56514. Fixing by reverting to have tag_defs.cc define the vars in a separate file and not just in the header. Why does this fix it - I have no idea... for some reason the c++ api dynamic linking madness doesn't like the inlined vars on the mac build specifically??? --------- Signed-off-by: dayshah <dhyey2019@gmail.com> Signed-off-by: Future-Outlier <eric901201@gmail.com>

[core] Cleanup metrics stuff + fix gcc >10 build

847969e

Signed-off-by: dayshah <dhyey2019@gmail.com>

dayshah added the go add ONLY when ready to merge, run all tests label Sep 14, 2025

fix cpp api

81c51ee

Signed-off-by: dayshah <dhyey2019@gmail.com>

dayshah marked this pull request as ready for review September 14, 2025 18:14

dayshah requested review from a team, SongGuyang, kfstorm and raulchen as code owners September 14, 2025 18:14

ray-gardener bot added the core Issues that should be addressed in Ray Core label Sep 14, 2025

dayshah added 2 commits September 15, 2025 01:56

fix cpp test

9bf4cd1

Signed-off-by: dayshah <dhyey2019@gmail.com>

fix cpp test

1ca4d74

Signed-off-by: dayshah <dhyey2019@gmail.com>

edoakes assigned can-anyscale Sep 15, 2025

can-anyscale self-requested a review September 15, 2025 20:16

can-anyscale reviewed Sep 15, 2025

View reviewed changes

dayshah added 3 commits September 17, 2025 22:35

revert cpp api

23dba6b

Signed-off-by: dayshah <dhyey2019@gmail.com>

format

20090df

Signed-off-by: dayshah <dhyey2019@gmail.com>

Merge branch 'master' into fix-build+metrics

c310324

Signed-off-by: dayshah <dhyey2019@gmail.com>

dayshah requested review from can-anyscale and edoakes September 18, 2025 05:39

edoakes approved these changes Sep 18, 2025

View reviewed changes

edoakes merged commit 2330b62 into ray-project:master Sep 18, 2025
5 checks passed

dayshah deleted the fix-build+metrics branch September 18, 2025 15:45

mattip mentioned this pull request Sep 19, 2025

Cannot build on Ubuntu 24.04 with gcc 14 #56710

Closed

israbbani added a commit that referenced this pull request Sep 23, 2025

Revert "[core] Cleanup metrics stuff + fix gcc >10 build (#56514)"

bf6715b

This reverts commit 2330b62.

dayshah added a commit to dayshah/ray that referenced this pull request Sep 25, 2025

[core] Fix cpp api mac breakage from ray-project#56514

22b22e6

Signed-off-by: dayshah <dhyey2019@gmail.com>

dayshah mentioned this pull request Sep 25, 2025

[core] Fix cpp api mac breakage from #56514 #56915

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Cleanup metrics stuff + fix gcc >10 build#56514

[core] Cleanup metrics stuff + fix gcc >10 build#56514
edoakes merged 7 commits intoray-project:masterfrom
dayshah:fix-build+metrics

dayshah commented Sep 14, 2025 •

edited

Loading

Uh oh!

edoakes commented Sep 15, 2025

Uh oh!

can-anyscale left a comment

Uh oh!

can-anyscale Sep 15, 2025

Uh oh!

dayshah Sep 16, 2025

Uh oh!

edoakes Sep 16, 2025

Uh oh!

can-anyscale Sep 17, 2025

Uh oh!

dayshah Sep 18, 2025

Uh oh!

edoakes Sep 18, 2025

Uh oh!

can-anyscale Sep 15, 2025

Uh oh!

dayshah Sep 16, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

dayshah commented Sep 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are these changes needed?

Uh oh!

edoakes commented Sep 15, 2025

Uh oh!

can-anyscale left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dayshah commented Sep 14, 2025 •

edited

Loading