BentoML

BentoML copied to clipboard

The currently pinned versions of Open Telemetry's packages are somewhat outdated, meaning that the dependencies they pull in are also somewhat outdated. In turn this can cause conflicts with other libraries.

I appreciate that Open Telemetry's packages aren't themselves great at their version spellings/matching, however it's not clear that BentoML pinning specific versions is the best solution.

A particular example of package version conflicts is that opentelemetry-proto==1.14.0 requires protobuf~=3.13, yet the most recent Google Cloud packages require protobuf >= 4.

@aarnphm do you have context on why we locked the otel dep versions?

Because different otlp packages have conflict dependencies. Also newer version of otlp also breaks our monitoring API, cc @bojiang

What conflicting dependencies?

What conflicting dependencies?

I believe it was protobuf, where opentelemetry-http-proto has a newer version of protobuf where other opentelemetry repo lock to an older protobuf version iirc

Looks like that's sorted now?

From a clean clone of this repo, with the following patch applied:

diff --git a/pyproject.toml b/pyproject.toml
index ca2909c9..d7e33c0f 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -39,14 +39,14 @@ dependencies = [
     # OpenTelemetry is the server dependencies, rather than SDK
     # Since there are discrepancies among API and instrumentation packages,
     # we should always pin the set version of Opentelemetry suite
-    "opentelemetry-api==1.14.0",
-    "opentelemetry-sdk==1.14.0",
-    "opentelemetry-exporter-otlp-proto-http==1.14.0",
-    "opentelemetry-instrumentation==0.35b0",
-    "opentelemetry-instrumentation-aiohttp-client==0.35b0",
-    "opentelemetry-instrumentation-asgi==0.35b0",
-    "opentelemetry-semantic-conventions==0.35b0",
-    "opentelemetry-util-http==0.35b0",
+    "opentelemetry-api",
+    "opentelemetry-sdk",
+    "opentelemetry-exporter-otlp-proto-http",
+    "opentelemetry-instrumentation",
+    "opentelemetry-instrumentation-aiohttp-client",
+    "opentelemetry-instrumentation-asgi",
+    "opentelemetry-semantic-conventions",
+    "opentelemetry-util-http",
     "packaging>=22.0",
     "pathspec",
     "pip-tools>=6.6.2",

Then in a fresh Python 3.7 virtualenv:

$ pip install -U pip setuptools wheel
$ pip install . protobuf

completes cleanly.

The package versions installed end up being:

$ pip list | grep -P '(opentel|proto)'
googleapis-common-protos                     1.59.0
opentelemetry-api                            1.17.0
opentelemetry-exporter-otlp-proto-http       1.17.0
opentelemetry-instrumentation                0.38b0
opentelemetry-instrumentation-aiohttp-client 0.38b0
opentelemetry-instrumentation-asgi           0.38b0
opentelemetry-proto                          1.17.0
opentelemetry-sdk                            1.17.0
opentelemetry-semantic-conventions           0.38b0
opentelemetry-util-http                      0.38b0
protobuf                                     4.22.3

I've put together a trivial PR (#3791) with approximately that change in case it's useful.

Hi, this will be addressed with the upcoming release.

@aarnphm Was this ever addressed? It looks to me like these are still pinned?

we already bumped to the latest version that we tested

p/s: The reason behind locking OTEL is that all of these instrument packages that we are using are maintained by OTEL community, and oftentimes time the transitive dependencies of the instruments are conflicted. Hence, the version locking.

There are a few things we need to consider when bumping the version:

• Monitoring feature from OTEL is experimental iirc, then we need to make sure it won't break our implementation
• Since these OTEL deps literally break every update (or that was our experience in the past few version, not sure what their policies now), it makes it very hard to us to just unlock version of OTEL
• Protobuf conflict (as a side effect of OTEL deps)

Since rust server is in the works, we will probably migrate tracing to rust (so we don't have to worry about runtime dependencies anymore)

fwiw, we probably just need to reimplement these instrument ourself using OTEL API so that we don't have this problem anymore.

Hopefully that makes sense.

@aarnphm please could BentoML bump its open telemetry packages to at least 0.41b0, which contains a fix for security issue https://github.com/advisories/GHSA-5rv5-6h4r-h22v.

@PeterJCLaw Will release a new version soon that includes #4233