1348375 - Re-enable flake8 "F401: module imported but unused"

Status: RESOLVED FIXED

(Tree Management :: Treeherder, enhancement, P2)

Description

[Ed Morley [:emorley]]

As part of the flake8/pyflakes update recently, this rule was disabled:
F401: module imported but unused

Since it generated errors like:

treeherder/client/__init__.py:3:1: F401 '.thclient.*' imported but unused
treeherder/client/thclient/__init__.py:1:1: F401 '.client.*' imported but unused
treeherder/client/thclient/__init__.py:2:1: F401 '.perfherder.*' imported but unused
treeherder/etl/tasks/__init__.py:1:1: F401 '.buildapi_tasks.*' imported but unused
treeherder/etl/tasks/__init__.py:2:1: F401 '.pulse_tasks.*' imported but unused
treeherder/etl/tasks/__init__.py:3:1: F401 '.classification_mirroring_tasks.*' imported but unused
treeherder/etl/tasks/__init__.py:4:1: F401 '.tasks.*' imported but unused
treeherder/perfalert/__init__.py:3:1: F401 '.perfalert.*' imported but unused

However it turns out this error code wasn't new, it's just that pyflakes now reports it for these lines in addition to:

F403: 'from module import *' used; unable to detect undefined names

...when previously it only reported F403 (which is already on the ignore list).
(It's not clear if this is a pyflakes bug or not.)

However F401 is actually important for cases we do want to catch, like:

tests/autoclassify/test_classify_failures.py:7:1: F401 'treeherder.model.models.TextLogErrorMetadata' imported but unused
tests/etl/test_job_loader.py:7:1: F401 'treeherder.model.models.Repository' imported but unused
tests/model/test_classified_failure.py:6:1: F401 'treeherder.model.models.FailureLine' imported but unused
tests/seta/conftest.py:2:1: F401 'django.utils.timezone' imported but unused
tests/seta/test_job_priorities.py:8:1: F401 'treeherder.seta.settings.SETA_LOW_VALUE_PRIORITY' imported but unused
tests/webapp/api/test_text_log_summary_lines.py:4:1: F401 'treeherder.model.models.TextLogError' imported but unused
treeherder/auth/backends.py:13:5: F401 'django.utils.encoding.smart_str as smart_bytes' imported but unused
treeherder/autoclassify/tasks.py:4:1: F401 'django.conf.settings' imported but unused
treeherder/autoclassify/tasks.py:6:1: F401 'treeherder.celery_app' imported but unused
treeherder/seta/analyze_failures.py:7:1: F401 'treeherder.etl.seta.valid_platform' imported but unused
treeherder/seta/job_priorities.py:10:1: F401 'treeherder.model.models.Repository' imported but unused
treeherder/seta/models.py:7:1: F401 'treeherder.model.models.Repository' imported but unused

Either way, we should re-enable it and either `#noqa` the import * cases, or else fix them.

Comment 1

[GitHub Autolander Bot]

Attachment: [treeherder] mozilla:flake8-unused-imports > mozilla:master

Comment 2

[Treeherder GitHub Bugbot]

Commits pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/4c66f383ef0863bb1cb7d03cc849d4e9390a53ef
Bug 1348375 - Remove unused imports

Fixes:

tests/autoclassify/test_classify_failures.py:7:1: F401 'treeherder.model.models.TextLogErrorMetadata' imported but unused
tests/etl/test_job_loader.py:7:1: F401 'treeherder.model.models.Repository' imported but unused
tests/model/test_classified_failure.py:6:1: F401 'treeherder.model.models.FailureLine' imported but unused
tests/seta/conftest.py:2:1: F401 'django.utils.timezone' imported but unused
tests/seta/test_job_priorities.py:8:1: F401 'treeherder.seta.settings.SETA_LOW_VALUE_PRIORITY' imported but unused
tests/webapp/api/test_text_log_summary_lines.py:4:1: F401 'treeherder.model.models.TextLogError' imported but unused
treeherder/auth/backends.py:13:5: F401 'django.utils.encoding.smart_str as smart_bytes' imported but unused
treeherder/autoclassify/tasks.py:4:1: F401 'django.conf.settings' imported but unused
treeherder/autoclassify/tasks.py:6:1: F401 'treeherder.celery_app' imported but unused
treeherder/perfalert/__init__.py:1:1: F401 '.perfalert.*' imported but unused
treeherder/seta/analyze_failures.py:7:1: F401 'treeherder.etl.seta.valid_platform' imported but unused
treeherder/seta/job_priorities.py:10:1: F401 'treeherder.model.models.Repository' imported but unused
treeherder/seta/models.py:7:1: F401 'treeherder.model.models.Repository' imported but unused

The seta migrations file change is due to the seta models no longer
depending on `model` (since the unnecessary `Repository` import has
been removed).

https://github.com/mozilla/treeherder/commit/63372232164bf09ae4378e07e196c407a0b29229
Bug 1348375 - Stop using wildcard imports in treeherder.etl.tasks

They only save a few characters and make the location of imported
functions less clear.

Fixes:

treeherder/etl/tasks/__init__.py:1:1: F401 '.buildapi_tasks.*' imported but unused
treeherder/etl/tasks/__init__.py:2:1: F401 '.pulse_tasks.*' imported but unused
treeherder/etl/tasks/__init__.py:3:1: F401 '.classification_mirroring_tasks.*' imported but unused
treeherder/etl/tasks/__init__.py:4:1: F401 '.tasks.*' imported but unused

https://github.com/mozilla/treeherder/commit/f4b37c026b2d6a386ebdcf885d6a473303d02c6a
Bug 1348375 - Stop using a wildcard import in treeherder.client

This import only affects internal treeherder usage, people using the
PyPI package import from the `thclient` subdirectory instead.

Fixes:

treeherder/client/__init__.py:1:1: F401 '.thclient.*' imported but unused

https://github.com/mozilla/treeherder/commit/7ea160559e7e9cab221dc34d3ac8d032cf23b55f
Bug 1348375 - Stop using a wildcard import in treeherder.perfalert

At some point we should probably just merge perfalert into Perfherder,
but for now let's avoid unnecessary wildcard imports to save a few
characters.

Fixes:

treeherder/perfalert/__init__.py:1:1: F401 '.perfalert.*' imported but unused

https://github.com/mozilla/treeherder/commit/395df0a5e9947eac61add3941c91f2424c9f0b03
Bug 1348375 - Mark treeherder.client.thclient wildcard imports as `noqa`

These are used by consumers of the package on PyPI, so whilst there are
probably better ways to structure the modules (including more contained
exports by using `__all__`), this is safer/less effort for now.

Fixes:

treeherder/client/thclient/__init__.py:1:1: F401 '.client.*' imported but unused
treeherder/client/thclient/__init__.py:2:1: F401 '.perfherder.*' imported but unused

https://github.com/mozilla/treeherder/commit/c1dfddc5f604b9948f712498f04569c1426f5782
Bug 1348375 - Re-enable flake8 rule F401

Since it's actually needed to catch unused imports, albeit it's a bit
more strict than it used to be, so required some fix-ups before it could
be re-enabled.

Comment 3

[GitHub Autolander Bot]

Attachment: [treeherder] mozilla:fix-celery-imports > mozilla:master

Comment 4

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/ae6cc36b6b5b0e7af68d2e9ad054da4b65469c54
Bug 1348375 - Re-add wildcard imports in treeherder.etl.tasks

Since it turns out Celery uses them during task auto-discovery.

This partially reverts commit 63372232164bf09ae4378e07e196c407a0b29229.

Comment 5

[Ed Morley [:emorley]]

Unfortunately the initial landing caused errors like:

app/worker_store_pulse_jobs.1: [2017-03-21 15:39:28,641: ERROR/MainProcess] Received unregistered task of type u'store-pulse-jobs'. 
app/worker_store_pulse_jobs.1: The message has been ignored and discarded. 
app/worker_store_pulse_jobs.1: Did you remember to import the module containing this task? 
app/worker_store_pulse_jobs.1: Or maybe you are using relative imports? 
app/worker_store_pulse_jobs.1: Please see http://bit.ly/gLye1c for more information. 
app/worker_store_pulse_jobs.1: The full contents of the message body was: 
app/worker_store_pulse_jobs.1: {u'chord': None, u'timelimit': [None, None], u'utc': True, u'expires': None, u'taskset': None, u'callbacks': None, u'args': [<SNIP>
app/worker_store_pulse_jobs.1: Traceback (most recent call last): 
app/worker_store_pulse_jobs.1:   File "/app/.heroku/python/lib/python2.7/site-packages/celery/worker/consumer.py", line 465, in on_task_received 
app/worker_store_pulse_jobs.1:     strategies[type_](message, body, 
app/worker_store_pulse_jobs.1: KeyError: u'store-pulse-jobs' 

Unfortunately these:
1) didn't result in any test or linter failures.
2) didn't appear in New Relic, so when I'd checked stage before performing the deploy, there were no errors to be seen.
3) resulted in loss of taskcluster jobs in the time before this was reverted (~45 mins), due to Celery's really unhelpful behaviour of discarding tasks that are unknown rather than re-queuing them.

Things we can do to avoid this in the future:
1) I've added a comment to the file explaining the slightly magical Celery auto-discover behaviour.
2) We should investigate more thorough testing of Celery behaviour (eg stopping using CELERY_ALWAYS_EAGER and having proper test workers a la http://docs.celeryq.org/en/latest/userguide/testing.html).
3) File a bug against New Relic to make the Python agent catch this exception properly.
4) See if the new beta New Relic alerts system can detect cases where no ingestion is occurring (we already have alerts for errors, but previously it's not been possible to detect lack of activity).
5) File a bug against Celery asking them to default to re-queuing unknown tasks (so at least there wouldn't be data loss).