Skip to content
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

Add type hints to opentelemetry-sdk and run as a part of tests #1608

Open
srikanthccv opened this issue Feb 14, 2021 · 18 comments · May be fixed by #3809
Open

Add type hints to opentelemetry-sdk and run as a part of tests #1608

srikanthccv opened this issue Feb 14, 2021 · 18 comments · May be fixed by #3809
Assignees
Labels
priority:p2 Issues that are less important than priority:p1 style triaged

Comments

@srikanthccv
Copy link
Member

srikanthccv commented Feb 14, 2021

Much of opentelemetry-sdk code doesn't have type hints. It would be great to add mypy type hints and run the tests similar to opentelemetry-api.

@codeboten codeboten added style priority:p2 Issues that are less important than priority:p1 labels Feb 18, 2021
@codeboten
Copy link
Contributor

Related to #773

@aabmass
Copy link
Member

aabmass commented Feb 22, 2021

I've looked into this a few times and there were really annoying issues with namespace packages and finding the code. Mypy 0.8xx (#1575) has made huge improvements there and I think it should be much cleaner now. Like you mentioned, there is a lot of untyped code and code with types that was added as a best effort without running mypy. I think we will need a really relaxed config or start with only a few SDK files and slowly work up.

It would be awesome if we could get a nicer pattern for tox/mypy/setuptools so any new packages we scaffold will be fully checked from the start

@dmolenda-sumo
Copy link
Contributor

I recently updated mypy to 0.812 version (#1705). Finding code inside packages is much easier right now, so I can take care of running mypy tests for opentelemetry-sdk. Could you assign me to this issue?

@github-actions
Copy link

github-actions bot commented May 2, 2021

This issue was marked stale due to lack of activity. It will be closed in 30 days.

@lzchen
Copy link
Contributor

lzchen commented May 3, 2021

@dmolenda-sumo
Are you still working on this?

@github-actions github-actions bot removed the backlog label May 4, 2021
@dmolenda-sumo
Copy link
Contributor

Yes, I do. I've been a little busy recently, but I hope to finish this task soon

@github-actions
Copy link

github-actions bot commented Jun 4, 2021

This issue was marked stale due to lack of activity. It will be closed in 30 days.

@lzchen lzchen added triaged and removed backlog labels Jun 4, 2021
@lzchen
Copy link
Contributor

lzchen commented Jun 4, 2021

@dmolenda-sumo
Any progress on this?

@aceberle
Copy link

aceberle commented Mar 22, 2022

It seems like Mypy is expecting there to be an __init__.py file in the opentelemetry directory

I've experimented with adding an empty __init__.py file to the root opentelemetry directory and it resolved all of the mypy typing issues that I was having with the opentelemetry libraries.

It seems like you would just need to add it to this location and everything would be fine:
https://github.com/open-telemetry/opentelemetry-python/tree/main/opentelemetry-api/src/opentelemetry

@aceberle
Copy link

Actually, there appear to be quite a few __init__.py files missing

And I think I'm beginning to see that there might be a problem with the fact that the main module name 'opentelemetry' is used in multiple installation packages, when it probably would have made more sense to have a separate root module name for each package (i.e. 'opentelemetryapi' for -api, 'opentelemetrysdk' for -sdk, etc.)

@srikanthccv
Copy link
Member Author

@aceberle
Copy link

aceberle commented Mar 23, 2022

Hi @srikanthccv, thank you for directing me to that information. So is it expected that anyone pulling opentelemetry into their project will need to run mypy with this --namespace-packages argument to avoid the import errors?

I noticed that the tox.ini file uses this argument that you pointed out with the mypy command during the automated tests:

mypy: mypy --install-types --non-interactive --namespace-packages --explicit-package-bases opentelemetry-api/src/opentelemetry/

@srikanthccv
Copy link
Member Author

Yes

@aceberle
Copy link

Thank you so much @srikanthccv, I added that to my configuration and now mypy is able to resolve all of the imports from opentelemetry-python!

@adriangb
Copy link
Contributor

I"m happy to do a bit of work on this but to be honest I've found working with the opentelemetry repos very hard. I have plenty of experience working with complex setups but I keep finding issues with these repos, like linting seemingly not running properly in CI or errors simply following the CONTRIBUTING.md instructions. Would one of the maintainers that's familiar with the setup be willing to spend an hour on a call going through things and maybe we can make PRs to fix these sorts of issues or clarify docs where that make sense?

@srikanthccv
Copy link
Member Author

This is one area we all agree that the current experience is really poor. I suspect, that even within the core team, we have found different ways of working around the problems. I want to see this improved. I spent some time in the past to replace our tooling in the hope of a better experience #3260 but that didn't materialize. I will be happy to get on a call but let me throw away my current setup and start fresh to see what are the common issues folks run into and help fix them.

@ocelotl
Copy link
Contributor

ocelotl commented Jan 15, 2024

@adriangb @srikanthccv were you able to realize that call? @adriangb do you need further help?

@adriangb
Copy link
Contributor

We did have a call, during it we found that formatting / linting wasn’t working at all (or wasn’t working for the sdk package, I don’t remember the exact details, maybe @srikanthccv does) so it was very productive!

That said this issues should remain open since the original request hasn’t been addressed. I personally would like to work on it but have not had time lately.

@aabmass aabmass linked a pull request Mar 22, 2024 that will close this issue
asasvari pushed a commit to asasvari/opentelemetry-python that referenced this issue Jul 13, 2024
Part of open-telemetry#1608

Addressing running mypy on opentelemetry-sdk iteratively so we don't have to make one big change addressing all mypy issues at once.
asasvari pushed a commit to asasvari/opentelemetry-python that referenced this issue Jul 13, 2024
Part of open-telemetry#1608

Addressing running mypy on opentelemetry-sdk iteratively so we don't have to make one big change addressing all mypy issues at once.
asasvari added a commit to asasvari/opentelemetry-python that referenced this issue Jul 13, 2024
Part of open-telemetry#1608

Addressing running mypy on opentelemetry-sdk iteratively so we don't have to make one big change addressing all mypy issues at once.
asasvari added a commit to asasvari/opentelemetry-python that referenced this issue Jul 13, 2024
Part of open-telemetry#1608

Addressing running mypy on opentelemetry-sdk iteratively so we don't have to make one big change addressing all mypy issues at once.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:p2 Issues that are less important than priority:p1 style triaged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants