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

Running mypy on SDK resources (following up #3995) #4053

Merged
merged 35 commits into from
Jul 30, 2024

Conversation

asasvari
Copy link
Contributor

@asasvari asasvari commented Jul 13, 2024

Description

This PR is a continuation of #3995
Quoting Medinat Lawal:
Part of #1608
Addressing running mypy on opentelemetry-sdk iteratively so we don't have to make one big change addressing all mypy issues at once.

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
  • Ran basic examples (async_context.py, basic_trace.py, logs/example.py) using local changes

Checklist:

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

@asasvari
Copy link
Contributor Author

pre-commit
pre-commit
pre-commit fails with the changes

[WARNING] Unstaged files detected.
[INFO] Stashing unstaged files to /home/asasvari/.cache/pre-commit/patch1720865016-69994.
black....................................................................Failed
- hook id: black
- files were modified by this hook

reformatted opentelemetry-sdk/src/opentelemetry/sdk/resources/__init__.py

All done! ✨ 🍰 ✨
1 file reformatted.

isort....................................................................Failed
- hook id: isort
- files were modified by this hook

Fixing /home/asasvari/workspace/ep24/sprint/opentelemetry-python/opentelemetry-sdk/src/opentelemetry/sdk/resources/__init__.py

flake8...................................................................Failed
- hook id: flake8
- exit code: 1

opentelemetry-sdk/src/opentelemetry/sdk/resources/__init__.py:67:1: F401 'typing.Any' imported but unused
opentelemetry-sdk/src/opentelemetry/sdk/resources/__init__.py:200:13: F821 undefined name 'resource_detectors'
opentelemetry-sdk/src/opentelemetry/sdk/resources/__init__.py:211:13: F821 undefined name 'resource_detectors'

[WARNING] Stashed changes conflicted with hook auto-fixes... Rolling back fixes...
[INFO] Restored changes from /home/asasvari/.cache/pre-commit/patch1720865016-69994.

Copy link

linux-foundation-easycla bot commented Jul 13, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@asasvari asasvari force-pushed the pr3995 branch 2 times, most recently from a8bce5a to 82a980b Compare July 13, 2024 10:59
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 asasvari changed the title Do not merge: this is just to re-test PR 3995 Running mypy on SDK resources (following up #3995) Jul 13, 2024
@asasvari
Copy link
Contributor Author

I performed more testing locally

PASS

  • tox -e mypy
  • tox -e lint
  • tox -e test-opentelemetry-api

FAIL

  • tox -e test-opentelemetry-sdk
=========================================== 207 failed, 261 passed in 13.47s ===========================================
WARNING  opentelemetry.sdk.metrics._internal.export:__init__.py:328 Cannot call collect on a MetricReader until it is registered on a MeterProvider
test-opentelemetry-sdk: exit 1 (13.83 seconds) /home/asasvari/workspace/ep24/sprint/opentelemetry-python> pytest /home/asasvari/workspace/ep24/sprint/opentelemetry-python/opentelemetry-sdk/tests pid=8471
  test-opentelemetry-sdk: FAIL code 1 (29.37=setup[0.05]+cmd[15.48,13.83] seconds)
  evaluation failed :( (29.49 seconds)

opentelemetry-api/src/opentelemetry/attributes/__init__.py Outdated Show resolved Hide resolved
opentelemetry-api/src/opentelemetry/attributes/__init__.py Outdated Show resolved Hide resolved
opentelemetry-api/src/opentelemetry/attributes/__init__.py Outdated Show resolved Hide resolved
opentelemetry-api/src/opentelemetry/attributes/__init__.py Outdated Show resolved Hide resolved
opentelemetry-api/src/opentelemetry/attributes/__init__.py Outdated Show resolved Hide resolved
@asasvari
Copy link
Contributor Author

tox mypy finds 4 errors for 33e35c28

Found 4 errors in 1 file (checked 30 source files)
opentelemetry-api/src/opentelemetry/attributes/__init__.py:152: error: Missing type parameters for generic type "dict"  [type-arg]
opentelemetry-api/src/opentelemetry/attributes/__init__.py:152: error: Missing type parameters for generic type "OrderedDict"  [type-arg]
opentelemetry-api/src/opentelemetry/attributes/__init__.py:187: error: Function is missing a type annotation for one or more arguments  [no-untyped-def]
opentelemetry-api/src/opentelemetry/attributes/__init__.py:198: error: Expression type contains "Any" (has type "dict[Any, Any] | OrderedDict[Any, Any]")  [misc]
  • test-opentelemetry-sdk and test-opentelemetry-api passes

@xrmx xrmx added Skip Changelog PRs that do not require a CHANGELOG.md entry and removed Skip Changelog PRs that do not require a CHANGELOG.md entry labels Jul 25, 2024
@xrmx
Copy link
Contributor

xrmx commented Jul 25, 2024

@asasvari Please also add a Changelog entry to a new Unreleased section

@asasvari asasvari requested a review from xrmx July 25, 2024 15:38
@xrmx
Copy link
Contributor

xrmx commented Jul 25, 2024

@asasvari Please rebase to fix CHANGELOG conflicts

CHANGELOG.md Outdated Show resolved Hide resolved
@xrmx xrmx requested review from xrmx and aabmass July 26, 2024 09:33
CHANGELOG.md Outdated Show resolved Hide resolved
@lzchen lzchen merged commit 51b7e4c into open-telemetry:main Jul 30, 2024
284 checks passed
@asasvari
Copy link
Contributor Author

@xrmx , @lzchen many thanks for the reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants