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

Pylint crashes if Django is not configured #277

Closed
ljodal opened this issue Jul 29, 2020 · 8 comments
Closed

Pylint crashes if Django is not configured #277

ljodal opened this issue Jul 29, 2020 · 8 comments

Comments

@ljodal
Copy link

ljodal commented Jul 29, 2020

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Author

ljodal commented Jul 30, 2020

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
Copy link
Contributor

atodorov commented Jul 30, 2020

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
Copy link
Contributor

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
Copy link
Member

carlio commented Jul 30, 2020

@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
Copy link
Contributor

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
Copy link
Contributor

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.

atodorov added a commit that referenced this issue Aug 4, 2020
if Django isn't configured, that is DJANGO_SETTINGS_MODULE isn't
available in the environment, then raise a RuntimeError with message

DJANGO_SETTINGS_MODULE required for resolving ForeignKey string
references, see Usage section in README!
atodorov added a commit that referenced this issue Aug 4, 2020
if Django isn't configured, that is DJANGO_SETTINGS_MODULE isn't
available in the environment, then raise a RuntimeError with message

DJANGO_SETTINGS_MODULE required for resolving ForeignKey string
references, see Usage section in README!
atodorov added a commit that referenced this issue Aug 4, 2020
if Django isn't configured, that is DJANGO_SETTINGS_MODULE isn't
available in the environment, then raise a RuntimeError with message

DJANGO_SETTINGS_MODULE required for resolving ForeignKey string
references, see Usage section in README!
carlio added a commit that referenced this issue Aug 24, 2020
…he `open()` pre-configuration. This allows the django settings module to be set from a command-line option or .pylintrc option as well as an environment variable (refs #286). This commit also uses default Django settings if none are specified, raising an error to the user to inform them that it is better to set it explicitly, but avoiding the need to raise a Fatal or RuntimeError if Django is not configured (refs #277 and #243)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants