Running mypy on sdk resources #773 #4360

prabhakarjuzgar · 2024-12-16T21:02:29Z

Description

Addressing running mypy on opentelemetry-sdk iteratively so we don't have to make one big change addressing all mypy issues at once.

Fixes # (issue)

Type of change

Run mypy against SDK resources

How Has This Been Tested?

I executed the following tox commands:

tox -e mypy
tox -e test-opentelemetry-sdk
tox -e test-opentelemetry-api

Checklist:

Followed the style guidelines of this project
Changelogs have been updated
Unit tests have been added
Documentation has been updated

xrmx

Are these changes need to type check sdk resources? We are already checking it AFAICS, per mypy: mypy --install-types --non-interactive --namespace-packages --explicit-package-bases opentelemetry-sdk/src/opentelemetry/sdk/resources in tox.ini.

CHANGELOG.md

xrmx · 2024-12-17T08:43:13Z

opentelemetry-sdk/src/opentelemetry/sdk/error_handler/__init__.py

            return None

        plugin_handled = False

-        error_handler_entry_points = entry_points(
+        error_handler_entry_points = entry_points(  # type: ignore


This can be typed, https://github.com/python/importlib_metadata/blob/main/importlib_metadata/__init__.py#L1039C42-L1039C43
Need to check if we are exporting it from our util module

It is - https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-api/src/opentelemetry/util/_importlib_metadata.py#L28

basedpyright: Import "opentelemetry.util._importlib_metadata" could not be resolved. This results in following errors when mypy is executed -

opentelemetry-sdk/src/opentelemetry/sdk/error_handler/init.py:119: error: Type of variable becomes "Any" due to an unfollowed import [no-any-unimported]
opentelemetry-sdk/src/opentelemetry/sdk/error_handler/init.py:119: error: Expression has type "Any" [misc]

@xrmx Please let me know if the above errors can be handled differently.

I mean something like this error_handler_entry_points : Entrypoints = entry_points(

This PR has more type ignores than actual typing improvements.

This is a tricky one to type hint because of the way https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-api/src/opentelemetry/util/_importlib_metadata.py is implemented.

@Kludex do you have any suggestions?

This PR is really only adding type: ignore, there isn't any benefit to do this.

My suggestion is for the project to fix the mypy setup in this project, or move to pyright.

I'm happy to help, if the maintainers are willing.

If you can help, please create an issue about the mypy setup.

opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/exemplar/exemplar_reservoir.py

prabhakarjuzgar · 2024-12-17T09:35:28Z

Are these changes need to type check sdk resources? We are already checking it AFAICS, per mypy: mypy --install-types --non-interactive --namespace-packages --explicit-package-bases opentelemetry-sdk/src/opentelemetry/sdk/resources in tox.ini.

This does not execute mypy on sdk/src folder. Will add mypy: mypy --install-types --non-interactive --namespace-packages --explicit-package-bases opentelemetry-sdk/src to tox.ini after fixing all the files.

Kludex

It may be nice to enable https://docs.astral.sh/ruff/rules/blanket-type-ignore/#blanket-type-ignore-pgh003 to make it easier to understand on review what's the purpose of the ignores @xrmx

CHANGELOG.md

opentelemetry-api/src/opentelemetry/util/_importlib_metadata.py

Kludex

More honest review done.

Kludex · 2025-01-07T10:27:58Z

opentelemetry-sdk/src/opentelemetry/sdk/error_handler/__init__.py

@@ -61,6 +61,7 @@ def _handle(self, error: Exception, *args, **kwargs):

 from abc import ABC, abstractmethod
 from logging import getLogger
+from typing import Optional


I'd rather prefer to use from __future__ import annotations on the top of the imports, and use pipe (|) annotation.

Kludex · 2025-01-07T10:28:43Z

opentelemetry-sdk/src/opentelemetry/sdk/error_handler/__init__.py

@@ -69,7 +70,7 @@ def _handle(self, error: Exception, *args, **kwargs):

 class ErrorHandler(ABC):
    @abstractmethod
-    def _handle(self, error: Exception, *args, **kwargs):
+    def _handle(self, error: Exception, *args, **kwargs) -> None:  # type: ignore[misc, no-untyped-def]


The no-untyped-def can be removed by properly type hint it:

Suggested change

def _handle(self, error: Exception, *args, **kwargs) -> None: # type: ignore[misc, no-untyped-def]

def _handle(self, error: Exception, *args: Any, **kwargs: Any) -> None:

Why is the misc there?

Kludex · 2025-01-07T10:28:51Z

opentelemetry-sdk/src/opentelemetry/sdk/error_handler/__init__.py

@@ -83,7 +84,7 @@ class _DefaultErrorHandler(ErrorHandler):
    """

    # pylint: disable=useless-return
-    def _handle(self, error: Exception, *args, **kwargs):
+    def _handle(self, error: Exception, *args, **kwargs) -> None:  # type: ignore[no-untyped-def]


Same as above.

Kludex · 2025-01-07T10:29:20Z

opentelemetry-sdk/src/opentelemetry/sdk/error_handler/__init__.py

        pass

    # pylint: disable=no-self-use
-    def __exit__(self, exc_type, exc_value, traceback):
-        if exc_value is None:
+    def __exit__(self, exc_type, exc_value, traceback) -> Optional[bool]:  # type: ignore[no-untyped-def]


Use type hints mentioned here: https://stackoverflow.com/a/58808179

And remove the type ignore.

Kludex · 2025-01-07T10:29:34Z

opentelemetry-sdk/src/opentelemetry/sdk/error_handler/__init__.py

-    def __exit__(self, exc_type, exc_value, traceback):
-        if exc_value is None:
+    def __exit__(self, exc_type, exc_value, traceback) -> Optional[bool]:  # type: ignore[no-untyped-def]
+        if exc_value is None:  # type: ignore


If above applied, the ignore is not needed.

Suggested change

if exc_value is None: # type: ignore

if exc_value is None:

Kludex · 2025-01-07T10:30:10Z

opentelemetry-sdk/src/opentelemetry/sdk/error_handler/__init__.py

            return None

        plugin_handled = False

-        error_handler_entry_points = entry_points(
+        error_handler_entry_points = entry_points(  # type: ignore[misc]


What's the misc error here?

Same question applies to the below.

Kludex · 2025-01-07T10:31:00Z

opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/exemplar/exemplar_reservoir.py

@@ -129,7 +129,7 @@ def collect(self, point_attributes: Attributes) -> Optional[Exemplar]:
            {
                k: v
                for k, v in self.__attributes.items()
-                if k not in point_attributes
+                if k not in point_attributes  # type: ignore[operator]


This seems valid. What values Attributes can assume?

Kludex · 2025-01-07T10:32:09Z

opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/exemplar/exemplar_reservoir.py

+    def __init__(self, size: int, **kwargs) -> None:  # type: ignore[no-untyped-def]
+        super().__init__(**kwargs)  # type: ignore[misc]


What's the signature of ExemplarReservoir?

Running mypy on sdk resources open-telemetry#773

3ab4dc7

prabhakarjuzgar requested a review from a team as a code owner December 16, 2024 21:02

Update changelog

c38154e

xrmx reviewed Dec 17, 2024

View reviewed changes

prabhakarjuzgar added 2 commits December 18, 2024 09:12

Remove unicode chars and unwanted ignore

5c6d89b

Remove unnecessary changes from CHANGELOG

fc6c6e5

Kludex reviewed Dec 18, 2024

View reviewed changes

Kludex reviewed Dec 20, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

prabhakarjuzgar and others added 4 commits December 20, 2024 13:28

Change importlib_metadata to importlib.metadata

822e841

Address review comment

192db0c

Use importlib_metadata if importlib.data is unavailable

853c00f

Merge branch 'main' into pr773

2ddbd55

xrmx reviewed Dec 23, 2024

View reviewed changes

opentelemetry-api/src/opentelemetry/util/_importlib_metadata.py Outdated Show resolved Hide resolved

prabhakarjuzgar added 4 commits December 23, 2024 18:10

Remove unnecessary changes

d42a650

do not change implementation

99f1ff4

do not change implementation final

825c727

suppress specific warnings

ce4c3a7

Kludex suggested changes Jan 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Running mypy on sdk resources #773 #4360

Running mypy on sdk resources #773 #4360

prabhakarjuzgar commented Dec 16, 2024

xrmx left a comment

xrmx Dec 17, 2024

prabhakarjuzgar Dec 17, 2024

prabhakarjuzgar Dec 17, 2024

prabhakarjuzgar Dec 18, 2024

xrmx Dec 18, 2024 •

edited

Loading

Kludex Dec 31, 2024

prabhakarjuzgar Jan 1, 2025

prabhakarjuzgar Jan 7, 2025

Kludex Jan 7, 2025

Kludex Jan 7, 2025

prabhakarjuzgar commented Dec 17, 2024

Kludex left a comment •

edited

Loading

Kludex left a comment

Kludex Jan 7, 2025

Kludex Jan 7, 2025

Kludex Jan 7, 2025

Kludex Jan 7, 2025

Kludex Jan 7, 2025

Kludex Jan 7, 2025

Kludex Jan 7, 2025

Kludex Jan 7, 2025

Kludex Jan 7, 2025

	def _handle(self, error: Exception, args, *kwargs) -> None: # type: ignore[misc, no-untyped-def]
	def _handle(self, error: Exception, args: Any, *kwargs: Any) -> None:

		def __init__(self, size: int, **kwargs) -> None: # type: ignore[no-untyped-def]
		super().__init__(**kwargs) # type: ignore[misc]

Running mypy on sdk resources #773 #4360

Are you sure you want to change the base?

Running mypy on sdk resources #773 #4360

Conversation

prabhakarjuzgar commented Dec 16, 2024

Description

Type of change

How Has This Been Tested?

Checklist:

xrmx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xrmx Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prabhakarjuzgar commented Dec 17, 2024

Kludex left a comment • edited Loading

Choose a reason for hiding this comment

Kludex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xrmx Dec 18, 2024 •

edited

Loading

Kludex left a comment •

edited

Loading