[Symbols]Exporting openceus for streaming outside

[ashione]

Why are these changes needed?

Opencenus symobls haven been exported in linux version of libcore_worker_library_java.so, but deleted from ray_exported_symbols.lds, which makes streaming macos test case failed.
This pr add this exporting record and rename raystreaming* stuff to rayinternal* that's a united entry to ray cpp.

Related issue number

ray-project/mobius#26

Checks

• I've run scripts/format.sh to lint the changes in this PR.
• I've included any doc changes needed for https://docs.ray.io/en/master/.
• I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
• Testing Strategy
  • Unit tests
  • Release tests
  • This PR is not tested :(

[qicosmos]

LGTM

[jovany-wang]

@mwtian CC

[scv119]

Exposing opencensus symbols from Ray seems wrong. Previously, we have been running into issues when grpc symbols are exported. Can we change the test so they don’t depend on ray’s open census symbols? Alternatively, can we rename those symbols so they became Ray specific?

[ashione]

Exposing opencensus symbols from Ray seems wrong. Previously, we have been running into issues when grpc symbols are exported. Can we change the test so they don’t depend on ray’s open census symbols? Alternatively, can we rename those symbols so they became Ray specific?

DO NOT USE ray deps sounds reasonable to us. But there is one exception : we must keep one same static var with ray if both ray and outside library use it. Actually opencensus have many static variables so we have to export them to other cpp library.

[mwtian]

Agree with Chen, non-Ray symbols should not be exported by Ray. Since opencensus symbols are not exported by Ray, it should be ok for mobius to statically link against the opencensus library on its own.

[ashione]

Agree with Chen, non-Ray symbols should not be exported by Ray. Since opencensus symbols are not exported by Ray, it should be ok for mobius to statically link against the opencensus library on its own.

Did you truly know why these symobls should be exported?

                 U __ZN10opencensus4tags6TagKey8RegisterENSt3__117basic_string_viewIcNS2_11char_traitsIcEEEE
                 U __ZN10opencensus5stats13StatsExporter10RemoveViewENSt3__117basic_string_viewIcNS2_11char_traitsIcEEEE

none of them are used in mobius, but why we need and export them to other module?

The deep reason is there are ton of misuse of static variables in ray header files.

1. ray/src/ray/stats/metric.h

   Line 34 in 3cb8585

   #include "ray/stats/tag_defs.h"

2. ray/src/ray/stats/metric.h

   Line 112 in 3cb8585

   virtual ~Metric() { opencensus::stats::StatsExporter::RemoveView(name_); }

we would be milking a bull if tried to settle the problem just let outside project imports library by self.

[mwtian]

I don't know the structure of Mobius very well, but not finding the opencensus symbols could be a result of reusing Ray's stats lib via build object, instead of bazel dependency. What is the reason to depend on stats_lib.so, instead of @com_github_ray_project_ray//:stats_lib? It seems a bit unusual.

There definitely can be issues in Ray usage of global variables. Would you be willing to make a fix for them and see if they fix Mobius' issue as well?

[ashione]

I don't know the structure of Mobius very well, but not finding the opencensus symbols could be a result of reusing Ray's stats lib via build object, instead of bazel dependency. What is the reason to depend on stats_lib.so, instead of @com_github_ray_project_ray//:stats_lib? It seems a bit unusual.

There definitely can be issues in Ray usage of global variables. Would you be willing to make a fix for them and see if they fix Mobius' issue as well?

It was and is this style to use it.
By the way, this's common way to decouple ray deps from other static libraries.

[ashione]

I don't know the structure of Mobius very well, but not finding the opencensus symbols could be a result of reusing Ray's stats lib via build object, instead of bazel dependency. What is the reason to depend on stats_lib.so, instead of @com_github_ray_project_ray//:stats_lib? It seems a bit unusual.

There definitely can be issues in Ray usage of global variables. Would you be willing to make a fix for them and see if they fix Mobius' issue as well?

It's static link to use @com_github_ray_project_ray//:stats_lib that create differents static varabiles between mobius and ray. Ray can not find any static varibles if mobius use in it own static link.

For example, there is A static var with 0 by default in opencensus.
If mobius records

opencensus::A = 10
``` in source code anywhere.
But ray still gets opencesnus::A equals to 0.

[mwtian]

If this is calling into Ray instead of reusing code from Ray, the 2nd approach of cleaning up ray/src/ray/stats/metric.h could work.

[ashione]

If this is calling into Ray instead of reusing code from Ray, the 2nd approach of cleaning up ray/src/ray/stats/metric.h could work.

Code polished for cleaning static variables misusing is long term issue, which is not my changes can do it.

According to common external usage guidelines, all of cpp symbols should be published via ray internal namespace that has been created in my previous PR.

[mwtian]

Then can the opencensus export be removed?

[ashione]

Then can the opencensus export be removed?

Opencensus exporting should be removed definitely if ray stats lib hidens all of opencensus static symbols.

[mwtian]

Here are some of the issues Ray had with exporting non-Ray symbols: #18870, #20132. Ray needs to avoid similar issues as much as possible.

ray/src/ray/stats/metric.h has not been a public header. Doing some work to support using it as an API into Ray seems reasonable. Alternatively, building with opencensus in Mobius may not prevent using Ray's Metric class.

@jovany-wang you may have more context here. What is your recommendation on this?

[ashione]

Here are some of the issues Ray had with exporting non-Ray symbols: #18870, #20132. Ray needs to avoid similar issues as much as possible.

ray/src/ray/stats/metric.h has not been a public header. Doing some work to support using it as an API into Ray seems reasonable. Alternatively, building with opencensus in Mobius may not prevent using Ray's Metric class.

@jovany-wang you may have more context here. What is your recommendation on this?

I just wanna to keep macos same with linux exported whatever decision ray main repo would make and how to manage library or exported api. This topic is out of scope of these changes. Actually it might not be done in this PR.

[jovany-wang]

@mwtian I agree that we should hide all non-Ray symbols at high level. But it's not related to what we change in this PR.
For this PR, I believe it's just aligns the symbols with linux's. @ashione doesn't introduce any new symbol to export.

[jovany-wang]

@ashione is making efforts on hiding non Ray symbols as well, but it should take some time. And I prefer to merge this PR as it doesn't introduce any new symbol.

[mwtian]

I understand opencensus is already exported in Linux builds via ray_version_script.lds. It would be unfortunate if another user runs into the symbol collision problem on Linux / MacOS. Is there timeline to remove the export? I'm out of office this week. Leaving this to @scv119 and @ericl to decide.

[scv119]

@ashione is making efforts on hiding non Ray symbols as well, but it should take some time.

@jovany-wang I fully understand this blocks the streaming work, but we also risk our user running into ABI incompatibility issues (again), which harms the project in the long run.
I'm OK merging this if we have a concrete timeline to fix it.

cc @iycheng @rkooo567 to get a quorum

[rkooo567]

Yeah the failure mode here is this pr will fix the issue and we won’t fix the root cause since we resolve the immediate issue. We should use all these issues as a chance to fix bad things in ray.

I am also fine merging it, but it’d be great to know concrete timeline regarding when you guys plan to fix the underlying problem

[ashione]

@jovany-wang @mwtian @scv119 @rkooo567 Any updates for it?

[ashione]

@ashione is making efforts on hiding non Ray symbols as well, but it should take some time.

@jovany-wang I fully understand this blocks the streaming work, but we also risk our user running into ABI incompatibility issues (again), which harms the project in the long run.

I'm OK merging this if we have a concrete timeline to fix it.

cc @iycheng @rkooo567 to get a quorum

I'll work on removing exporting opencensus symbols. Perhaps it would be finished in next week.

[jovany-wang]

tests:test_output failure also happens on master branch.

Thank you guys, and I'm merging this.