Pylint crashes if Django is not configured

[ljodal]

Hi there, starting with version 2.2.0 I'm seeing a crash if django is not properly configured. I'm assuming this is originating from #243. This happens even if the string foreign key is resolvable without using the Django runtime.

While I kinda understand that you'd expect to have a Django configured while running pylint, it's not always the case and in some cases it might be hard (like in an editor).

Traceback (most recent call last):
  File "<snip>/.venv/lib/python3.7/site-packages/astroid/__init__.py", line 93, in _inference_tip_cached
    return iter(_cache[func, node])
KeyError: (<function infer_key_classes at 0x1113d5b00>, <Call l.162 at 0x11b5f3650>)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<snip>/.venv/bin/pylint", line 8, in <module>
    sys.exit(run_pylint())
  File "<snip>/.venv/lib/python3.7/site-packages/pylint/__init__.py", line 22, in run_pylint
    PylintRun(sys.argv[1:])
  File "<snip>/.venv/lib/python3.7/site-packages/pylint/lint/run.py", line 344, in __init__
    linter.check(args)
  File "<snip>/.venv/lib/python3.7/site-packages/pylint/lint/pylinter.py", line 871, in check
    self.get_ast, self._iterate_file_descrs(files_or_modules)
  File "<snip>/.venv/lib/python3.7/site-packages/pylint/lint/pylinter.py", line 904, in _check_files
    self._check_file(get_ast, check_astroid_module, name, filepath, modname)
  File "<snip>/.venv/lib/python3.7/site-packages/pylint/lint/pylinter.py", line 930, in _check_file
    check_astroid_module(ast_node)
  File "<snip>/.venv/lib/python3.7/site-packages/pylint/lint/pylinter.py", line 1063, in check_astroid_module
    ast_node, walker, rawcheckers, tokencheckers
  File "<snip>/.venv/lib/python3.7/site-packages/pylint/lint/pylinter.py", line 1107, in _check_astroid_module
    walker.walk(ast_node)
  File "<snip>/.venv/lib/python3.7/site-packages/pylint/utils/ast_walker.py", line 75, in walk
    self.walk(child)
  File "<snip>/.venv/lib/python3.7/site-packages/pylint/utils/ast_walker.py", line 75, in walk
    self.walk(child)
  File "<snip>/.venv/lib/python3.7/site-packages/pylint/utils/ast_walker.py", line 75, in walk
    self.walk(child)
  [Previous line repeated 7 more times]
  File "<snip>/.venv/lib/python3.7/site-packages/pylint/utils/ast_walker.py", line 72, in walk
    callback(astroid)
  File "<snip>/.venv/lib/python3.7/site-packages/pylint/checkers/base.py", line 781, in visit_call
    self._check_inferred_class_is_abstract(inferred, node)
  File "<snip>/.venv/lib/python3.7/site-packages/pylint/checkers/base.py", line 798, in _check_inferred_class_is_abstract
    abstract_methods = _has_abstract_methods(inferred)
  File "<snip>/.venv/lib/python3.7/site-packages/pylint/checkers/base.py", line 385, in _has_abstract_methods
    return len(utils.unimplemented_abstract_methods(node)) > 0
  File "<snip>/.venv/lib/python3.7/site-packages/pylint/checkers/utils.py", line 819, in unimplemented_abstract_methods
    inferred = safe_infer(obj)
  File "<snip>/.venv/lib/python3.7/site-packages/pylint/checkers/utils.py", line 1119, in safe_infer
    value = next(infer_gen)
  File "<snip>/.venv/lib/python3.7/site-packages/astroid/decorators.py", line 132, in raise_if_nothing_inferred
    yield next(generator)
  File "<snip>/.venv/lib/python3.7/site-packages/astroid/decorators.py", line 96, in wrapped
    res = next(generator)
  File "<snip>/.venv/lib/python3.7/site-packages/astroid/bases.py", line 136, in _infer_stmts
    for inferred in stmt.infer(context=context):
  File "<snip>/.venv/lib/python3.7/site-packages/astroid/node_classes.py", line 357, in infer
    return self._explicit_inference(self, context, **kwargs)
  File "<snip>/.venv/lib/python3.7/site-packages/astroid/__init__.py", line 95, in _inference_tip_cached
    result = func(*args, **kwargs)
  File "<snip>/.venv/lib/python3.7/site-packages/pylint_django/transforms/foreignkey.py", line 103, in infer_key_classes
    module_name = _module_name_from_django_model_resolution(model_name, module_name)
  File "<snip>/.venv/lib/python3.7/site-packages/pylint_django/transforms/foreignkey.py", line 46, in _module_name_from_django_model_resolution
    django.setup()
  File "<snip>/.venv/lib/python3.7/site-packages/django/__init__.py", line 19, in setup
    configure_logging(settings.LOGGING_CONFIG, settings.LOGGING)
  File "<snip>/.venv/lib/python3.7/site-packages/django/conf/__init__.py", line 76, in __getattr__
    self._setup(name)
  File "<snip>/.venv/lib/python3.7/site-packages/django/conf/__init__.py", line 61, in _setup
    % (desc, ENVIRONMENT_VARIABLE))
django.core.exceptions.ImproperlyConfigured: Requested setting LOGGING_CONFIG, but settings are not configured. You must either define the environment variable DJANGO_SETTINGS_MODULE or call settings.configure() before accessing settings.

pip freeze:

Adyen==3.0.0
aioredis==1.3.1
amqp==2.6.0
apipkg==1.5
appdirs==1.4.4
appnope==0.1.0
asgiref==3.2.10
astroid==2.4.2
async-timeout==3.0.1
attrs==19.3.0
autobahn==20.6.2
autograd==1.3
Automat==20.2.0
backcall==0.2.0
bcrypt==3.1.7
billiard==3.6.3.0
black==19.10b0
bokeh==2.1.1
cached-property==1.5.1
cachetools==4.1.1
celery==4.4.6
certifi==2020.6.20
cffi==1.14.0
channels==2.4.0
channels-redis==2.4.2
chardet==3.0.4
click==7.1.2
constantly==15.1.0
coverage==5.2
croniter==0.3.34
cryptography==2.9.2
cssselect==1.1.0
cssutils==1.0.2
curlify==2.2.1
dacite==1.5.1
daphne==2.5.0
datadog==0.38.0
ddtrace==0.40.0
decorator==4.4.2
defusedxml==0.6.0
dill==0.3.2
Django==3.0.8
django-appconf==1.0.4
django-bootstrap3==14.1.0
django-compressor==2.4
django-countries==6.1.2
django-debug-toolbar==2.2
django-environ==0.4.5
django-form-utils==1.0.3
django-groove==0.4.6
django-helpscout==0.6.3
django-ratelimit==3.0.1
django-redis==4.12.1
django-sendgrid-v5==0.8.1
django-stubs==1.5.0
django-webpack-loader==0.7.0
djangorestframework==3.11.0
dkimpy==1.0.4
dnspython==1.16.0
elasticsearch==6.8.1
execnet==1.7.1
Fabric3==1.14.post1
facebook-business==7.0.3
flake8==3.8.3
flake8-bugbear==20.1.4
freezegun==0.3.15
future==0.18.2
fuzzywuzzy==0.18.0
geographiclib==1.50
geopy==2.0.0
github3.py==1.3.0
google-api-core==1.21.0
google-auth==1.19.2
google-auth-oauthlib==0.4.1
google-cloud-core==1.3.0
google-cloud-pubsub==1.7.0
google-cloud-storage==1.30.0
google-crc32c==0.1.0
google-resumable-media==0.7.0
googleads==24.1.0
googleapis-common-protos==1.52.0
googletrans==2.4.0
grpc-google-iam-v1==0.12.3
grpcio==1.30.0
gunicorn==20.0.4
hiredis==1.1.0
hyperlink==19.0.0
idna==2.10
implicit==0.4.2
importlib-metadata==1.7.0
incremental==17.5.0
intervaltree==3.0.2
ipython==7.16.1
ipython-genutils==0.2.0
isodate==0.6.0
isort==4.3.21
jedi==0.17.1
Jinja2==2.11.2
jwcrypto==0.7
kombu==4.6.11
lazy-object-proxy==1.4.3
libcst==0.3.8
libsass==0.20.0
Lifetimes==0.11.3
logstash-python-formatter==0.1.2
lxml==4.5.2
MarkupSafe==1.1.1
maxminddb==1.5.4
mccabe==0.6.1
mock==4.0.2
more-itertools==8.4.0
msgpack==0.6.2
mypy==0.770
mypy-extensions==0.4.3
natsort==7.0.1
netaddr==0.8.0
numpy==1.18.3
oauthlib==3.1.0
ortools==7.7.7810
packaging==20.4
pandas==0.25.3
paramiko==2.7.1
parso==0.7.0
pathspec==0.8.0
pexpect==4.8.0
phonenumberslite==8.12.6
pickleshare==0.7.5
Pillow==7.2.0
pluggy==0.13.1
premailer==3.7.0
prompt-toolkit==3.0.5
protobuf==3.12.2
psycopg2==2.8.5
ptyprocess==0.6.0
py==1.9.0
pyasn1==0.4.8
pyasn1-modules==0.2.8
pycodestyle==2.6.0
pycountry==20.7.3
pycparser==2.20
pyflakes==2.2.0
Pygments==2.6.1
PyHamcrest==2.0.2
PyJWT==1.7.1
pylint==2.5.3
pylint-django==2.2.0
pylint-plugin-utils==0.6
PyNaCl==1.4.0
pyOpenSSL==19.1.0
pyparsing==2.4.7
pyproj==2.6.1.post1
pysaml2==6.1.0
pytest==5.4.3
pytest-cov==2.10.0
pytest-django==3.9.0
pytest-forked==1.2.0
pytest-mock==3.2.0
pytest-socket==0.3.5
pytest-xdist==1.34.0
python-barcode==0.11.0
python-dateutil==2.8.1
python-http-client==3.2.7
python-Levenshtein==0.12.0
python-memcached==1.59
pytz==2020.1
PyYAML==5.3.1
rcssmin==1.0.6
redis==3.5.3
regex==2020.7.14
requests==2.24.0
requests-mock==1.8.0
requests-oauthlib==1.3.0
requests-toolbelt==0.9.1
responses==0.10.15
rjsmin==1.1.0
rsa==4.6
scipy==1.5.1
sendgrid==6.4.3
sentry-sdk==0.16.1
service-identity==18.1.0
six==1.15.0
sortedcontainers==2.2.2
sqlparse==0.3.1
starkbank-ecdsa==1.0.0
stripe==2.49.0
suds-py3==1.4.1.0
tenacity==6.2.0
text-unidecode==1.3
toml==0.10.1
tornado==6.0.4
tqdm==4.47.0
traitlets==4.3.3
twilio==6.44.1
Twisted==20.3.0
txaio==20.4.1
typed-ast==1.4.1
typing-extensions==3.7.4.2
typing-inspect==0.6.0
ua-parser==0.10.0
uritemplate==3.0.1
urllib3==1.25.9
user-agents==2.1
vine==1.3.0
wcwidth==0.2.5
whitenoise==5.1.0
wrapt==1.11.2
xlrd==1.2.0
XlsxWriter==1.2.9
xmltodict==0.12.0
zeep==3.4.0
zipp==3.1.0
zope.interface==5.1.0

[atodorov]

Hi there, starting with version 2.2.0 I'm seeing a crash if django is not properly configured. I'm assuming this is originating from #243.

That's because Django needs its settings, see the docs:
https://github.com/PyCQA/pylint-django#usage

This happens even if the string foreign key is resolvable without using the Django runtime.

File "/.venv/lib/python3.7/site-packages/pylint_django/transforms/foreignkey.py", line 103, in infer_key_classes
module_name = _module_name_from_django_model_resolution(model_name, module_name)

Your FK reference is specified as a string so pylint-django tries to resolve it using the Django utilities and fails with missing settings. This happens here:

https://github.com/PyCQA/pylint-django/blob/master/pylint_django/transforms/foreignkey.py#L102

In case we get an error we'll try to fallback and append .models to the string but that's it. There's no option to first try resolving by doing the append and only if that fails try asking Django to figure it out.

This whole thing exists because Django doesn't follow its own practices to have app_name.models.ModelName but instead it is django.something.otherthing.app_name.models.ModelName - that's the Python module pylint needs to figure out. And based on my experience these are the majority of the cases for many apps, not only Django.

While I kinda understand that you'd expect to have a Django configured while running pylint, it's not always the case and in some cases it might be hard (like in an editor).

If your editor doesn't allow you to configure the pylint command then how does it know to run pylint-django as a plugin ? If this is an actual fact then I'm curious to know what exactly do you use for development.

OTOH IDEs (not plain editors) always allow you to configure the command which gets executed, although this might be cumbersome sometimes. Disclaimer: I'm not a fan of IDEs and use Makefile to configure how I run things.

@carlio, @alejandro-angulo - while the above LookupError exception can be extended to also include ImproperlyConfigured I'm not confident we should go this way. If we start ignoring the missing configuration then I suspect there will be a bunch of false results for the unresolved FK references and a lot more bugs reported by people who haven't configured their environment. What's your opinion ?

And FTR we've spent too much time dealing with this issue of FK references as strings that I'm starting to regret we even worked on it in the first place. It is usually a code smell like previously discussed in #243 (comment).

[alejandro-angulo]

I agree that ignoring ImproperlyConfigured might cause more headaches. Would be nice if we could only enable Django module resolution if a flag is set but I'm not sure there's an easy way to do that with pylint plugins.

[ljodal]

Hi again, and thanks for the replies!

While I understand that using string-references is not ideal, we are in a situation where we have a Django project with hundreds of models that have circular references to each other. The easiest way to deal with this is to use string-references, as python doesn't like circular imports.

I understand that handling this in a pylint plugin is be hard (we've had issues with this for a long time and I've already tried to fork pylint-django to find a solution), but just plain crashing seems like a really bad developer experience. Wouldn't it be better to catch the error and emit a warning or something like that?

We've had pylint running with pylint-django for a long time with the exact same configuration and it just started crashing on 2.2.0. It seems to me like with these changes DJANGO_SETTINGS_MODULE is required to be set, even if you'd normally not need that in your dev environment? Our setup has a default in manage.py, but I guess there's no other magic that could make pylint pick up that.

[atodorov]

I agree that ignoring ImproperlyConfigured might cause more headaches. Would be nice if we could only enable Django module resolution if a flag is set but I'm not sure there's an easy way to do that with pylint plugins.

What's the difference of a flag vs ENV variable ? Could be an if condition to check whether or not ENV is available though.

Edit: this will lead to the situation where people skip over the details in the documentation and string resolution stops working for them in some cases, so not very different from what I've described earlier.

[atodorov]

I understand that handling this in a pylint plugin is be hard (we've had issues with this for a long time and I've already tried to fork pylint-django to find a solution), but just plain crashing seems like a really bad developer experience. Wouldn't it be better to catch the error and emit a warning or something like that?

I'm not a fan of this, check-out this talk for more context https://www.youtube.com/watch?v=nCGBgI1MNwE. You get a warning which people will ignore then start asking question why this and why that and etc. Not ideal either so why bother ? If it crashes at least you know something's up.

We've had pylint running with pylint-django for a long time with the exact same configuration and it just started crashing on 2.2.0. It seems to me like with these changes DJANGO_SETTINGS_MODULE is required to be set,

Yes, it is a required. That's why the documentation has been updated although it doesn't say explicitly that it is required. But it is there on the command line. But nothing is stopping you to pass project.settings.test as the config for example. That's what I have in many of my own projects.

Our setup has a default in manage.py, but I guess there's no other magic that could make pylint pick up that.

Correct, the defaults in manage.py is just a glue layer for Django.

[carlio]

@atodorov I agree, having just a warning means that some people just ignore it then act confused when behaviour isn't as documented then come to the comments. My main learning (and regret!) from prospector is that if you try to make it so anyone can configure it to their environment, you end up needing to support many many different environment types which requires a lot of overhead in terms of issues. I'd recommend a much more simple "because you require feature X, you must do Y" error (or fatal) and leave it at that. As long as the error has a clear message with instructions (or a link to the relavent piece of the docs) I think that's the best solution.

[alejandro-angulo]

What's the difference of a flag vs ENV variable ? Could be an if condition to check whether or not ENV is available though.

Edit: this will lead to the situation where people skip over the details in the documentation and string resolution stops working for them in some cases, so not very different from what I've described earlier.

I was thinking that a flag would require an explicit action from the user to enable (similar to the additional migrations checker plugin). But then, like you said, there will inevitably be some people that don't read the documentation.

Checking if the environment variable is set sounds like a good solution to me. It should work as expected for existing users (no change in behavior if the variable is not set) and I think it would require minimal changes to the documentation (just an update to indicate that Django's resolution will be used if the variable is set).

My main learning (and regret!) from prospector is that if you try to make it so anyone can configure it to their environment, you end up needing to support many many different environment types which requires a lot of overhead in terms of issues.

Good counterpoint to checking for an environment variable. My only concern then is that maybe 2.2.0 should have been 3.0.0, since it introduced a backwards incompatible change.

[atodorov]

I'd recommend a much more simple "because you require feature X, you must do Y" error (or fatal) and leave it at that. As long as the error has a clear message with instructions (or a link to the relavent piece of the docs) I think that's the best solution.

@carlio this is simple enough and sounds good to me. And I can bump the version to 3.0, even remove 2.2.0 from PyPI to make it more obvious something big has changed.