Remove dependency on Tornado?

[obi1kenobi]

I'd like to propose that the tornado dependency is removed from install_requires and moved to the extras_require to make it conditional, even if #83 is being worked on in parallel.

I am working on a project that uses opentracing_instrumentation and jaeger_client, but does not use tornado. However, due to the dependencies on tornado in these two packages, I am forced to install tornado and specifically tornado>=4.1,<6 due to this package's dependency on it, since jaeger_client depends on tornado>=4.3 with no upper bound.

This causes an unfortunate problem, where even though my package doesn't rely on tornado, I still get DeprecationWarning telling me that my package will stop working in Python 3.9. To reproduce:

requirements.txt

opentracing==2.3.0
opentracing-instrumentation==3.3.1
jaeger-client==4.3.0

repro.py

try:
    import jaeger_client.config
except DeprecationWarning as e:
    print(e)
else:
    raise AssertionError(
        "Deprecation warning not raised, are you sure you ran this with the '-Werror' flag? "
        "Expected command: python -Werror repro.py"
    )

Executing in the environment created from that requirements.txt file:

$ python --version
Python 3.7.7

$ python -Werror repro.py 
Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated since Python 3.3,and in 3.9 it will stop working

While I import jaeger_client and not opentracing_instrumentation here, the bound on tornado<6 comes from opentracing_instrumentation and not jaeger_client, and that bound is the immediate cause of the DeprecationWarning.

This is related to, but not fully encompassed by, jaegertracing/jaeger-client-python#235.

Thank you for considering this, and for working on this project in general.

[kaushall]

Any updates on this?

[ottaviohartman]

This and #83 are more important with the new pip resolver, which is stricter for incompatible deps.

[sdanbury]

Happy to go ahead and create a PR for this if the core contributors are on-board with the idea. Let me know.

[yurishkuro]

You could try it, I don't think this is a trivial change. May also require ditching python2.

[sdanbury]

Interesting, thanks for getting back to me. Having looked a bit deeper, looks a bit more involved than I first expected.

[nikolak]

Personally I have a pipeline that scans all dependencies, and due to CVE-2020-28476 and this library not supporting tornado 6 it keeps complaining that there's insecure dependencies. Not a major thing, but perhaps another point/reason for either upgrading or dropping the dependency as described in the original proposal.

[jkgenser]

I'm running a jaeger client on celery workers and I see the workers logging span reported. Based on peoples' experience, is it likely that the tornado IOLoop is not playing well with celery and therefore resulting in spans not getting reported?

[yurishkuro]

@jkgenser celery workers are often implemented as forked processes. See https://github.com/jaegertracing/jaeger-client-python#wsgi-multi-processing-fork2

[jkgenser]

@yurishkuro thanks for getting back to me. Yeah I am initializing jaeger with worker process init signal so in the prefork pool this runs after the forking. Not experiencing deadlocks in my application at least, but maybe I'm experiencing deadlocks with jaeger-clients IOLoop...

https://docs.celeryproject.org/en/stable/userguide/signals.html#worker-process-init