-
Notifications
You must be signed in to change notification settings - Fork 649
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SDK namespace package #55
Conversation
to allow access to both opentelemetry.api and opentelemetry.sdk via namespace packages.
and make SDK package depend on API.
@@ -11,24 +11,32 @@ deps = | |||
mypy: mypy~=0.711 | |||
|
|||
setenv = | |||
PYTHONPATH={toxinidir}/opentelemetry-api/src/ | |||
mypy: MYPYPATH={env:PYTHONPATH} | |||
mypy: MYPYPATH={toxinidir}/opentelemetry-api/src/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting PYTHONPATH
instead of installing the package was a kludge. It may be possible to get mypy to pass without setting MYPYPATH
here, but just installing the packages didn't seem to do it.
|
||
changedir = | ||
test: opentelemetry-api/tests | ||
test-api: opentelemetry-api/tests | ||
test-sdk: opentelemetry-sdk/tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may not want separate test targets in the long run, but this was the easiest way to get test discovery to work since opentelemetry
isn't a package (with an __init__.py
) anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to remove opentelemetry/__init__.py
after we workaround the pylint discovery issue #37 (comment).
commands_pre = | ||
test-api: pip install -e {toxinidir}/opentelemetry-api | ||
test-sdk: pip install -e {toxinidir}/opentelemetry-api | ||
test-sdk: pip install -e {toxinidir}/opentelemetry-sdk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to install these as regular deps so they're not reinstalled on each run. There may be another better way to do this.
|
||
commands = | ||
py37-mypy: mypy opentelemetry-api/src/opentelemetry/ | ||
mypy: mypy --namespace-packages opentelemetry-api/src/opentelemetry/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See python/mypy#1645, mypy fails to find opentelemetry.api
without this flag.
This is different from my understanding. We've discussed that given users are only going to use APIs in their code, it makes more sense to let them use Regarding the With what we have in the master branch today, installing/uninstall api/sdk package should work:
|
The problem is that the API package takes over the % pip install -e opentelemetry-sdk; python -c "from opentelemetry import sdk"
Obtaining file:///Users/libc/src/opentelemetry-python/opentelemetry-sdk
Installing collected packages: opentelemetry-sdk
Running setup.py develop for opentelemetry-sdk
Successfully installed opentelemetry-sdk
% pip install -e opentelemetry-api; python -c "from opentelemetry import sdk"
Obtaining file:///Users/libc/src/opentelemetry-python/opentelemetry-api
Installing collected packages: opentelemetry-api
Running setup.py develop for opentelemetry-api
Successfully installed opentelemetry-api
Traceback (most recent call last):
File "<string>", line 1, in <module>
ImportError: cannot import name 'sdk' from 'opentelemetry' (/Users/libc/src/opentelemetry-python/opentelemetry-api/src/opentelemetry/__init__.py) |
This is due to the fact that we haven't removed |
Got it, removed the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Add Propagation interface * minor doc fix * Rename interface Propagation to Propagator
This PR adds a stub tracer implementation that depends on the API.
Doing this required some package surgery: moving the
opentelemetry
package inopentelemetry-api
toopentelemetry.api
and losingopentelemetry/__init__
modules, making both API and SDK PEP 420 namespace packages.The result is that you can now install both packages without either clobbering the other and causing import errors.
@reyang let me know if this matches your grand vision for the package structure.