Propagates more ImportErrors during autodiscovery #8632

johnjameswhitman · 2023-11-10T22:17:45Z

Problem

The gist of the problem is that the current implementation of autodiscovering tasks will hide ImportErrors that originate outside of importlib.import_module.

For details see discussion in #8620.

Solution

Narrow the caught-exceptions to ones that:

Are ModuleNotFoundError (which is a subclass of ImportError that importlib seems to raise these days), and
Have a name attribute that matches the task being "guessed" (previously the code would default to the task's name if the exception lacked a name attribute).

Also, refactored the tests into what seem like the specific edge cases find_related_module is trying to handle.

johnjameswhitman · 2023-11-10T22:18:25Z

celery/loaders/base.py

@@ -253,20 +253,26 @@ def find_related_module(package, related_name):
    # Django 1.7 allows for specifying a class name in INSTALLED_APPS.
    # (Issue #2248).
    try:
+        # Return package itself when no related_name.


I dropped some notes to myself in these comments to try and figure out what each of these conditions was trying to do. Happy to remove if you don't want to clutter the code.

johnjameswhitman · 2023-11-10T22:23:54Z

t/unit/app/test_loaders.py

+    def test_find_related_module__when_existent_package_parent_but_not_related_name(self):
+        with patch('importlib.import_module') as imp:
+            first_import = ModuleNotFoundError(name='foo.bar')
+            second_import = ModuleNotFoundError(name='foo.tasks')
+            imp.side_effect = [first_import, second_import]
+            assert base.find_related_module('foo.bar', 'tasks') is None
+            imp.assert_any_call('foo.bar')
+            imp.assert_any_call('foo.tasks')


This is a weird case -- I'm not sure if find_related_module is actually meant to handle this or not, so can remove it if not.

johnjameswhitman · 2023-11-10T22:24:54Z

t/unit/app/test_loaders.py

+
+    def test_find_related_module__when_existent_package_parent_and_related_name(self):
+        with patch('importlib.import_module') as imp:
+            first_import = ModuleNotFoundError(name='foo.BarApp')  # Ref issue #2248


It seems like this behavior was added to match some edge case w/ how Django apps present themselves for auto-discovery (see #2248).

codecov · 2023-11-10T23:23:29Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3723009) 87.33% compared to head (347b310) 87.34%.
Report is 14 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8632      +/-   ##
==========================================
+ Coverage   87.33%   87.34%   +0.01%     
==========================================
  Files         148      148              
  Lines       18515    18519       +4     
  Branches     3163     3164       +1     
==========================================
+ Hits        16170    16176       +6     
+ Misses       2060     2059       -1     
+ Partials      285      284       -1

Flag	Coverage Δ
unittests	`87.31% <100.00%> (+0.01%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

auvipy

thanks for handling this and digging deep into the code history! we will wait to have review/feedback from several maintainers so it might need some more time then expected.

In the mean time, it would be great if you can figure out missing test coverage or improve it further. I'm not sure this part of code has Integration test or not and if it is possible to add Intg tests or not. If you can check that can be added it will be really awesome

johnjameswhitman · 2023-11-13T18:15:23Z

Fixed the coverage issue. I don't think I saw integration tests but will double-check.

auvipy · 2023-11-14T05:04:43Z

CI failures are network issues. pypy passed after retry. now retried cpython3.12 again

auvipy · 2023-11-16T09:00:53Z

celery/loaders/base.py

        import_exc_name = getattr(e, 'name', module_name)
-        if import_exc_name is not None and import_exc_name != module_name:


can we make sure this change / removal won't create any regression?

Will do -- I should have some more time to look at this next week.

will appreciate this to be done today.... but if you need some more time that is completely fine and we can merge this after 5.3.6 release of celery?

Dropped an integration test in -- LMK how that looks 🙏

that look good!! thanks

johnjameswhitman · 2023-11-20T23:37:47Z

t/integration/test_loader.py

+    def test_autodiscovery(self, manager):
+        # Arrange
+        expected_package_name, _, module_name = __name__.rpartition('.')
+        unexpected_package_name = 'nonexistent.package.name'


This actually caught a bug I fixed in 3e405b1.

auvipy

can you please take a look into the following test failure? although I will try too re run the test to see if it pass. but fixing it would be great

=================================== FAILURES ===================================
________________________ test_tasks.test_task_accepted _________________________

self = <t.integration.test_tasks.test_tasks object at 0x7fc7c05b21c0>
manager = <celery.contrib.testing.manager.Manager object at 0x7fc7be18f820>
sleep = 1

  @flaky
  def test_task_accepted(self, manager, sleep=1):
      r1 = sleeping.delay(sleep)
      sleeping.delay(sleep)

  manager.assert_accepted([r1.id])

t/integration/test_tasks.py:409:

celery/contrib/testing/manager.py:150: in assert_accepted
return self.assert_task_worker_state(
celery/contrib/testing/manager.py:186: in assert_task_worker_state
return self.wait_for(
celery/contrib/testing/manager.py:81: in wait_for
return self.retry_over_time(
celery/contrib/testing/manager.py:107: in retry_over_time
return retry_over_time(*args, **kwargs)
.tox/3.8-integration-redis/lib/python3.8/site-packages/kombu/utils/functional.py:318: in retry_over_time
return fun(*args, **kwargs)

self = <celery.contrib.testing.manager.Manager object at 0x7fc7be18f820>
fun = <bound method ManagerMixin.is_accepted of <celery.contrib.testing.manager.Manager object at 0x7fc7be18f820>>
args = (['267f60e6-783a-49c5-a41f-4b34ec2f2908'],), kwargs = {'timeout': 0.5}
res = False

  def true_or_raise(self, fun, *args, **kwargs):
      res = fun(*args, **kwargs)
      if not res:

      raise Sentinel()

E celery.contrib.testing.manager.Sentinel

celery/contrib/testing/manager.py:208: Sentinel
------------------------------ Captured log call -------------------------------
INFO celery.worker.strategy:strategy.py:161 Task t.integration.tasks.sleeping[c3ad399a-a7b8-4766-a0c7-6d58201187f7] received
INFO celery.worker.strategy:strategy.py:161 Task t.integration.tasks.sleeping[fd71157f-8c08-4b28-81c9-9da73a18f9e5] received
INFO celery.app.trace:trace.py:131 Task t.integration.tasks.sleeping[c3ad399a-a7b8-4766-a0c7-6d58201187f7] succeeded in 1.0021785389999422s: None
INFO celery.app.trace:trace.py:131 Task t.integration.tasks.sleeping[fd71157f-8c08-4b28-81c9-9da73a18f9e5] succeeded in 1.0021406060000118s: None
INFO celery.worker.request:request.py:496 Discarding revoked task: t.integration.tasks.retry_unpickleable[66fecb16-57d6-4a53-9c06-9c457ccf3cb5]
------------------------------ Captured log call -------------------------------
INFO celery.worker.strategy:strategy.py:161 Task t.integration.tasks.sleeping[761953e3-17ce-4461-9f90-d0e6faf3a316] received
INFO celery.worker.strategy:strategy.py:161 Task t.integration.tasks.sleeping[97c69a23-d98e-456d-8043-4ea0b365a7aa] received
INFO celery.app.trace:trace.py:131 Task t.integration.tasks.sleeping[761953e3-17ce-4461-9f90-d0e6faf3a316] succeeded in 1.0021477600000708s: None
INFO celery.app.trace:trace.py:131 Task t.integration.tasks.sleeping[97c69a23-d98e-456d-8043-4ea0b365a7aa] succeeded in 1.0022280179998688s: None
------------------------------ Captured log call -------------------------------
INFO celery.worker.strategy:strategy.py:161 Task t.integration.tasks.sleeping[e5eb6331-eb5e-453e-bae0-aa9165ce2dc5] received
INFO celery.worker.strategy:strategy.py:161 Task t.integration.tasks.sleeping[17e36c16-8f74-415c-ba83-a98bda23d729] received
INFO celery.app.trace:trace.py:131 Task t.integration.tasks.sleeping[e5eb6331-eb5e-453e-bae0-aa9165ce2dc5] succeeded in 1.0022641999998996s: None
INFO celery.app.trace:trace.py:131 Task t.integration.tasks.sleeping[17e36c16-8f74-415c-ba83-a98bda23d729] succeeded in 1.0021502090000922s: None
------------------------------ Captured log call -------------------------------
INFO celery.worker.strategy:strategy.py:161 Task t.integration.tasks.sleeping[85f129f4-0425-4778-952e-907b38940e41] received
INFO celery.worker.strategy:strategy.py:161 Task t.integration.tasks.sleeping[44e5e5e1-170d-418d-97fa-21d813ed584c] received
INFO celery.app.trace:trace.py:131 Task t.integration.tasks.sleeping[85f129f4-0425-4778-952e-907b38940e41] succeeded in 1.0023080529999788s: None
INFO celery.app.trace:trace.py:131 Task t.integration.tasks.sleeping[44e5e5e1-170d-418d-97fa-21d813ed584c] succeeded in 1.0021153309999136s: None
------------------------------ Captured log call -------------------------------
INFO celery.worker.strategy:strategy.py:161 Task t.integration.tasks.sleeping[1f82ba9c-792d-4bad-bf0a-1403836fde26] received
INFO celery.worker.strategy:strategy.py:161 Task t.integration.tasks.sleeping[ae43d6b8-adfe-47b7-8caf-6d58c02ef704] received
INFO celery.app.trace:trace.py:131 Task t.integration.tasks.sleeping[1f82ba9c-792d-4bad-bf0a-1403836fde26] succeeded in 1.0012457650000215s: None
INFO celery.app.trace:trace.py:131 Task t.integration.tasks.sleeping[ae43d6b8-adfe-47b7-8caf-6d58c02ef704] succeeded in 1.001963686000181s: None
------------------------------ Captured log call -------------------------------
INFO celery.worker.strategy:strategy.py:161 Task t.integration.tasks.sleeping[267f60e6-783a-49c5-a41f-4b34ec2f2908] received
INFO celery.worker.strategy:strategy.py:161 Task t.integration.tasks.sleeping[74b0356c-02c3-470e-a1c7-bec26c9fde5b] received
INFO celery.app.trace:trace.py:131 Task t.integration.tasks.sleeping[267f60e6-783a-49c5-a41f-4b34ec2f2908] succeeded in 1.002038981999931s: None
INFO celery.app.trace:trace.py:131 Task t.integration.tasks.sleeping[74b0356c-02c3-470e-a1c7-bec26c9fde5b] succeeded in 1.0021865470000648s: None
=============================== warnings summary ===============================
t/integration/test_canvas.py: 39 warnings
/home/runner/work/celery/celery/celery/canvas.py:2283: CPendingDeprecationWarning: task_allow_error_cb_on_chord_header=False is pending deprecation in a future release of Celery.
Please test the new behavior by setting task_allow_error_cb_on_chord_header to True and report any concerns you might have in our issue tracker before we make a final decision regarding how errbacks should behave when used with chords.
warnings.warn(

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================== short test summary info ============================
FAILED t/integration/test_tasks.py::test_tasks::test_task_accepted - celery.c...

johnjameswhitman added 2 commits November 10, 2023 17:13

Refactors find_related_module tests.

f991ce3

Narrows exception catching.

3ee96f3

johnjameswhitman commented Nov 10, 2023

View reviewed changes

johnjameswhitman added 2 commits November 10, 2023 17:19

Makes a narrower assertion.

295a8d5

Cleans up test name.

696c43c

johnjameswhitman commented Nov 10, 2023

View reviewed changes

auvipy requested review from auvipy and a team November 11, 2023 05:30

auvipy reviewed Nov 11, 2023

View reviewed changes

auvipy added this to the 5.4 milestone Nov 12, 2023

johnjameswhitman added 2 commits November 13, 2023 11:27

Tries to address coverage miss.

9a01273

Cleans up comment.

3982e4d

johnjameswhitman requested a review from auvipy November 13, 2023 18:14

auvipy modified the milestones: 5.4, 5.3.x Nov 14, 2023

auvipy mentioned this pull request Nov 16, 2023

Celery v5.3.6 release #8642

Closed

10 tasks

auvipy requested changes Nov 16, 2023

View reviewed changes

johnjameswhitman added 3 commits November 20, 2023 16:25

Fixes typo.

d15c5f0

Adds integration test.

559dcd4

Fixes bug on ModuleNotFoundError.name when fails early.

3e405b1

johnjameswhitman commented Nov 20, 2023

View reviewed changes

Defaults getattr to None.

347b310

auvipy requested changes Nov 21, 2023

View reviewed changes

auvipy approved these changes Nov 21, 2023

View reviewed changes

auvipy merged commit 3ba50e4 into celery:main Nov 21, 2023
32 checks passed

auvipy added the Component: Django label Nov 21, 2023

auvipy added the PR Type: Bugfix label Nov 21, 2023

johnjameswhitman mentioned this pull request Nov 22, 2023

Re-raise ModuleNotFoundError unless for guessed task #8660

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propagates more ImportErrors during autodiscovery #8632

Propagates more ImportErrors during autodiscovery #8632

johnjameswhitman commented Nov 10, 2023 •

edited

johnjameswhitman Nov 10, 2023

johnjameswhitman Nov 10, 2023

johnjameswhitman Nov 10, 2023

codecov bot commented Nov 10, 2023 •

edited

auvipy left a comment

johnjameswhitman commented Nov 13, 2023

auvipy commented Nov 14, 2023 •

edited

auvipy Nov 16, 2023

johnjameswhitman Nov 17, 2023

auvipy Nov 20, 2023

johnjameswhitman Nov 20, 2023

auvipy Nov 21, 2023

johnjameswhitman Nov 20, 2023

auvipy Nov 21, 2023

auvipy left a comment

		import_exc_name = getattr(e, 'name', module_name)
		if import_exc_name is not None and import_exc_name != module_name:

Propagates more ImportErrors during autodiscovery #8632

Propagates more ImportErrors during autodiscovery #8632

Conversation

johnjameswhitman commented Nov 10, 2023 • edited

Problem

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 10, 2023 • edited

Codecov Report

auvipy left a comment

Choose a reason for hiding this comment

johnjameswhitman commented Nov 13, 2023

auvipy commented Nov 14, 2023 • edited

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

auvipy left a comment

Choose a reason for hiding this comment

johnjameswhitman commented Nov 10, 2023 •

edited

codecov bot commented Nov 10, 2023 •

edited

auvipy commented Nov 14, 2023 •

edited