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

recreate_module: set __spec__ to the new module #7773

Merged
merged 2 commits into from Sep 24, 2022

Conversation

skshetry
Copy link
Contributor

The recreated module has __spec__ set to None, due to which find_spec throws ValueError.

Reading PEP-451 and Python's docs for __spec__, it seems that __spec__ can be None in some cases but generally it should be set.

Here's the issue that this tries to fix:

>>> from importlib.util import find_spec
>>> find_spec("celery")
ModuleSpec(name='celery', loader=<_frozen_importlib_external.SourceFileLoader object at 0x7fadf901ea10>, origin='/home/user/projects/iterative/celery/celery/__init__.py', submodule_search_locations=['/home/user/projects/iterative/celery/celery'])
>>> import celery
>>> find_spec("celery")
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Input In [4], in <cell line: 0>()
----> 1 find_spec("celery")

File <frozen importlib.util>:114, in find_spec(name, package)

ValueError: celery.__spec__ is None

I noticed this when investigating failure in pylint/astroid, see PyCQA/pylint#7488 and PyCQA/astroid#1802.

@codecov
Copy link

codecov bot commented Sep 22, 2022

Codecov Report

Base: 89.75% // Head: 89.60% // Decreases project coverage by -0.14% ⚠️

Coverage data is based on head (d36cb9b) compared to base (34533ab).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7773      +/-   ##
==========================================
- Coverage   89.75%   89.60%   -0.15%     
==========================================
  Files         138      128      -10     
  Lines       17007    15860    -1147     
  Branches     2515     2346     -169     
==========================================
- Hits        15264    14212    -1052     
+ Misses       1491     1421      -70     
+ Partials      252      227      -25     
Flag Coverage Δ
unittests 89.59% <100.00%> (-0.15%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
celery/local.py 92.13% <100.00%> (+0.02%) ⬆️
celery/bin/celery.py
celery/result.py
celery/concurrency/__init__.py
celery/events/__init__.py
celery/security/__init__.py
celery/beat.py
celery/worker/__init__.py
celery/app/__init__.py
celery/__init__.py
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@skshetry
Copy link
Contributor Author

The test failure looks unrelated.

@auvipy
Copy link
Member

auvipy commented Sep 23, 2022

yes, can add some tests for the changes proposed?

@auvipy auvipy added this to the 5.3 milestone Sep 23, 2022
@skshetry
Copy link
Contributor Author

skshetry commented Sep 23, 2022

I can add something like follows, but I am not sure if it's worth it considering it's only used once:

from importlib.util import find_spec

class test_celery_import:
    def test_import_celery(self, monkeypatch):
        import celery

        monkeypatch.delitem(sys.modules, "celery", raising=False)
        spec = find_spec("celery")
        assert spec

        assert celery.__spec__ == spec
        assert find_spec("celery") == spec

@auvipy
Copy link
Member

auvipy commented Sep 23, 2022

some test is better the no test, so lets add the test as well

@skshetry
Copy link
Contributor Author

Thanks, I have added one.

@auvipy auvipy merged commit 6ea687b into celery:master Sep 24, 2022
@skshetry skshetry deleted the set-spec branch September 24, 2022 05:50
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.

None yet

2 participants