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.

Sign up for GitHub

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

Jump to bottom

Use importlib.metadata from the standard library on Python 3.8+ #1086

Merged
merged 2 commits into from
Dec 3, 2019
Merged

Use importlib.metadata from the standard library on Python 3.8+ #1086

merged 2 commits into from
Dec 3, 2019

Conversation

hroncok
Copy link
Contributor

@hroncok hroncok commented Aug 20, 2019

Note that I've tried to run tox, but was getting some version conflicts in amqp, missing test deps and later this failure:

self = <kombu.transport.pyro.Transport object at 0x7fe36537e0a0>

    def driver_version(self):
>       return pyro.__version__
E       AttributeError: 'NoneType' object has no attribute '__version__'

kombu/transport/pyro.py:119: AttributeError

I don't think it is related, so I've decided not to investigate further.

@codecov
Copy link

codecov bot commented Aug 20, 2019

Codecov Report

Merging #1086 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1086      +/-   ##
==========================================
+ Coverage   88.62%   88.62%   +<.01%     
==========================================
  Files          64       64              
  Lines        6689     6692       +3     
  Branches      805      805              
==========================================
+ Hits         5928     5931       +3     
  Misses        668      668              
  Partials       93       93
Impacted Files Coverage Δ
kombu/utils/compat.py 90.32% <100%> (+0.49%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a97495...7913343. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 20, 2019 •
edited
Loading

Codecov Report

Merging #1086 into master will decrease coverage by 0.2%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1086      +/-   ##
==========================================
- Coverage   88.82%   88.62%   -0.21%     
==========================================
  Files          65       64       -1     
  Lines        6785     6692      -93     
  Branches      815      805      -10     
==========================================
- Hits         6027     5931      -96     
- Misses        665      668       +3     
  Partials       93       93
Impacted Files Coverage Δ
kombu/utils/compat.py 90.32% <100%> (+0.49%) ⬆️
kombu/transport/redis.py 90.44% <0%> (-0.73%) ⬇️
kombu/transport/virtual/base.py 97.47% <0%> (-0.38%) ⬇️
kombu/asynchronous/hub.py 79.54% <0%> (-0.16%) ⬇️
kombu/utils/functional.py 98.57% <0%> (-0.06%) ⬇️
kombu/connection.py 98.75% <0%> (-0.03%) ⬇️
kombu/transport/azureservicebus.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70868ec...034d095. Read the comment docs.

auvipy
auvipy reviewed Aug 20, 2019
View reviewed changes
tox.ini Outdated Show resolved Hide resolved
auvipy
auvipy reviewed Aug 20, 2019
View reviewed changes
kombu/utils/compat.py
import importlib_metadata

try:
from importlib import metadata as importlib_metadata
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this import fails in python 3.7

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why it is in a try block.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not try import_metadata first?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To prefer the standard library module if it is there, over the backport library.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, kombu 4.6 ain't gonna get python 3.8 support. and 3.8 isn't stable yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

@hroncok hroncok force-pushed the importlib_metadata branch from 7913343 to c750395 Compare August 20, 2019 17:42
@auvipy auvipy added this to the 4.6.0 milestone Aug 20, 2019
matteius
matteius reviewed Aug 22, 2019
View reviewed changes
kombu/utils/compat.py
try:
from importlib import metadata as importlib_metadata
except ImportError:
import importlib_metadata
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is still going to cause an issue for eventlet and therefore open stack folks (because they use eventlet). Celery does support eventlet. Concurrent thread: #1082

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch Matt

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually though -- if we ensure eventlet users try the fix that @davidszotten mentioned "eventlet 0.25.1 has been released with a fix for the pathlib issue" maybe this code would be safer.

@auvipy auvipy modified the milestones: 4.6.0, 4.7 Aug 23, 2019
auvipy
auvipy requested changes Dec 3, 2019
View reviewed changes
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you rebase?

@hroncok
Copy link
Contributor Author

hroncok commented Dec 3, 2019

I can try tomorrow.

@auvipy auvipy modified the milestones: 4.7, 4.6.0 Dec 3, 2019
@auvipy auvipy merged commit 97e8876 into celery:master Dec 3, 2019
@hroncok hroncok deleted the importlib_metadata branch December 3, 2019 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matteius matteius matteius left review comments

@clokep clokep clokep left review comments

@auvipy auvipy auvipy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.6.0
Development

Successfully merging this pull request may close these issues.
4 participants
@hroncok @matteius @clokep @auvipy