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

Disable automatically detecting the use of frameworks in Prospector #1719

Closed
wants to merge 3 commits into from

Conversation

bouweandela
Copy link
Member

@bouweandela bouweandela commented Sep 9, 2022

Description

Disable automatically detecting prospector plugins that we do not use.

Link to documentation: https://prospector.landscape.io/en/master/profiles.html#libraries-used-and-autodetect

Closes #1703


Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.


To help with the number pull requests:

@codecov
Copy link

codecov bot commented Sep 9, 2022

Codecov Report

Merging #1719 (ce74e93) into main (62b5c30) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1719   +/-   ##
=======================================
  Coverage   91.47%   91.47%           
=======================================
  Files         206      206           
  Lines       11204    11204           
=======================================
  Hits        10249    10249           
  Misses        955      955           

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

@bouweandela bouweandela marked this pull request as ready for review September 9, 2022 08:48
@bouweandela bouweandela changed the title Disable autodetect Disable automatically detecting the use of frameworks in Prospector Sep 9, 2022
Copy link
Contributor

@schlunma schlunma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Please make sure to add a label before merging 🚀

@valeriupredoi
Copy link
Contributor

one sec, I want to run a GA testy-test on it

@bouweandela
Copy link
Member Author

😄 Do you have a suggestion for a label? I couldn't find any that matches.

@valeriupredoi
Copy link
Contributor

#bearatadinnerparty 🐻

@valeriupredoi valeriupredoi added installation Installation problem bear at a dinner party Something very unexpected labels Sep 12, 2022
@schlunma
Copy link
Contributor

😄 Do you have a suggestion for a label? I couldn't find any that matches.

Not really, also thought of the good old bear 🐻 😃

@valeriupredoi
Copy link
Contributor

awesome job @bouweandela - ran a set of GA tests just to make sure Django is unchained (doesn't get picked up, +1 if you get the movie reference haha) 🍺

@schlunma
Copy link
Contributor

Wait...The GH test you showed lists pylint-django:

Run conda list django
# packages in environment at /usr/share/miniconda3/envs/esmvalcore:
#
# Name                    Version                   Build  Channel
pylint-django             2.5.3                    pypi_0    pypi

Shouldn't this PR prevent this?

@valeriupredoi
Copy link
Contributor

indeed pylint-django is still there, but not django itself as prospector doesn't bring it in anymore. Not 100% sure who brings in pylint-django but at least it's not the hefty Django package (whoever brings it in it's after pip install, so maybe even prospector?)

@schlunma
Copy link
Contributor

But was django itself ever pulled in? It does not appear in latest GH action tests...

@valeriupredoi
Copy link
Contributor

yes, big hefty Django comes in brought in by prospector (cheers @zklaus for pointing that out, I thought Django was brought in by pylint-django)

@valeriupredoi
Copy link
Contributor

alas, having said all that, reading the documentation that Bouwe linked from the PR description, it seems that now prospector shouldn't bring in the plugin-django either so now I'm starting to get confusado

@schlunma
Copy link
Contributor

But why does it not appear in the GH action tests? And if pylint-django is still there, isn't there the chance that we still get these wrong Codacy issues?

@valeriupredoi
Copy link
Contributor

But why does it not appear in the GH action tests? And if pylint-django is still there, isn't there the chance that we still get these wrong Codacy issues?

it does, but only after pip installing, when prospector gets installed; and yes, those issues were from the plugin complaining Django wasn't properly installed right? Well, now it's not at all installed anymore so I wonder if it'll complain anymore 🤣

@valeriupredoi
Copy link
Contributor

so now Django was not configured. will probably turn into Looking for Django, seems to be chained still. 😆

@valeriupredoi
Copy link
Contributor

so here's the thing - I'm not sure we can get rid of that pylint-django dep brought in by prospector. I did an experiment whereby I didn't install prospector while pip installing it (removed it from our PyPi setup), and am trying to install it from conda-forge instead: it brings in all this crap:

  Install:
───────────────────────────────────────────────────────────────────────────────────────

  + asgiref                   3.5.2  pyhd8ed1ab_0     conda-forge/noarch       Cached
  + astroid                  2.12.9  py310hff52083_0  conda-forge/linux-64     Cached
  + colorama                  0.4.5  pyhd8ed1ab_0     conda-forge/noarch       Cached
  + dill                    0.3.5.1  pyhd8ed1ab_0     conda-forge/noarch       Cached
  + django                    4.1.1  py310hff52083_0  conda-forge/linux-64     Cached
  + dodgy                     0.2.1  py_0             conda-forge/noarch       Cached
  + flake8                    3.8.4  py_0             conda-forge/noarch       Cached
  + flake8-polyfill           1.0.2  py_0             conda-forge/noarch       Cached
  + importlib-metadata       4.11.4  py310hff52083_0  conda-forge/linux-64     Cached
  + isort                    5.10.1  pyhd8ed1ab_0     conda-forge/noarch       Cached
  + lazy-object-proxy         1.7.1  py310h5764c6d_1  conda-forge/linux-64     Cached
  + logilab-common            1.7.3  py_0             conda-forge/noarch       Cached
  + mccabe                    0.6.1  py_1             conda-forge/noarch       Cached
  + mypy_extensions           0.4.3  py310hff52083_5  conda-forge/linux-64     Cached
  + pep8-naming              0.10.0  pyh9f0ad1d_0     conda-forge/noarch       Cached
  + platformdirs              2.5.2  pyhd8ed1ab_1     conda-forge/noarch       Cached
  + prospector              1.4.1.1  pyhd8ed1ab_0     conda-forge/noarch       Cached
  + pycodestyle               2.6.0  pyh9f0ad1d_0     conda-forge/noarch       Cached
  + pydocstyle                6.1.1  pyhd8ed1ab_0     conda-forge/noarch       Cached
  + pyflakes                  2.2.0  pyh9f0ad1d_0     conda-forge/noarch       Cached
  + pylint                   2.15.2  pyhd8ed1ab_0     conda-forge/noarch       Cached
  + pylint-celery               0.3  py_1             conda-forge/noarch       Cached
  + pylint-django            2.0.11  py_0             conda-forge/noarch       Cached
  + pylint-flask                0.6  py_0             conda-forge/noarch       Cached
  + pylint-plugin-utils         0.7  pyhd8ed1ab_0     conda-forge/noarch       Cached
  + requirements-detector       0.7  py310hff52083_3  conda-forge/linux-64     Cached
  + setoptconf                0.3.0  pyhd8ed1ab_0     conda-forge/noarch       Cached
  + snowballstemmer           2.2.0  pyhd8ed1ab_0     conda-forge/noarch       Cached
  + sqlparse                  0.4.2  pyhd8ed1ab_0     conda-forge/noarch       Cached
  + tomli                     2.0.1  pyhd8ed1ab_0     conda-forge/noarch       Cached
  + tomlkit                  0.11.4  pyha770c72_0     conda-forge/noarch       Cached
  + typing                 3.10.0.0  pyhd8ed1ab_0     conda-forge/noarch       Cached
  + typing_extensions         4.3.0  pyha770c72_0     conda-forge/noarch       Cached
  + wrapt                    1.14.1  py310h5764c6d_0  conda-forge/linux-64     Cached
  + zipp                      3.8.1  pyhd8ed1ab_0     conda-forge/noarch       Cached

On the other hand, it's possible the Django from conda be correctly configured?

@valeriupredoi
Copy link
Contributor

Ok - the crux of this PR is not really working: prospector still installs all the pylint plugins that nobody needs:

pylint-celery             0.3                      pypi_0    pypi
pylint-django             2.5.3                    pypi_0    pypi
pylint-flask              0.6                      pypi_0    pypi

I suggest we open an issue on the prospector GH, prob more on the pylint side actually and tell them it's not quite working out what they thought the --no-autodetect should do. I am trying to not get myself into the PyCQA GH area, so anyone else? 😁

@schlunma
Copy link
Contributor

Maybe we should merge this and test if the Codacy problem disappears? E.g., here?

Maybe the installation of the plugin cannot be disabled, but the the actual use of it can...

@valeriupredoi
Copy link
Contributor

Maybe we should merge this and test if the Codacy problem disappears? E.g., here?

Maybe the installation of the plugin cannot be disabled, but the the actual use of it can...

totally do! I'm just annoyed we get crap in our env we don't need, celery above all else, I hate celery 🥕 😁

@zklaus
Copy link

zklaus commented Sep 13, 2022

I am afraid this will not help. What this PR should achieve is that Prospector doesn't run pylint-django, but it is still installed as a dependency. If it is installed via conda-forge, that will also pull in django because of this.

The problem is that the Codacy problem did not come from the Prospector tool, but directly from the Pylint tool, so we need to prevent Pylint from running pylint-django, not only Prospector. I was surprised though that Pylint did run it, since the Codacy configuration defers to our Pylint config file which makes no mention of the plugin and I could also not find anything about autodetection in Pylint itself, though I didn't look very thoroughly yet.

@valeriupredoi
Copy link
Contributor

@zklaus good points! There is this old PR pylint-dev/pylint-django#137 that decoupled Django from Pylint, but it seems they're still chained somehow

@zklaus
Copy link

zklaus commented Sep 13, 2022

Good find, @valeriupredoi. It seems conda-forge is overdepending. I added an issue at conda-forge/pylint-django-feedstock#39.

However, that doesn't solve our issue with Pylint running the plugin outside of Prospector.

@valeriupredoi
Copy link
Contributor

we should definitely tell the prospector folk about this. I did the following experiment: I installed prospector with pip install prospector --no-deps then I installed one by one the packages required for it to run - nowhere in that list were pylint_celery, _flask, and _django (but just the vanilla pylint). After that, prospector runs fine if and only if one runs it like prospctor your-terrible-code.py --no-autodetect otherwise without the --no-autodetect opt, it gets stuck running the autodetect.py module - my guess is that the prospector devs are a bit lazy at making the no autodetection option run fine from the get going, so they're still keeping those pylint plugins as dependencies (but don't quote me on that 😁 )

@bouweandela
Copy link
Member Author

bouweandela commented Sep 16, 2022

Plugins are indeed disabled in the pylint configuration file:

load-plugins=

so I'm not sure why Codacy is still running pylint-django if it uses our configuration file.

@bouweandela
Copy link
Member Author

I will close this now because it seems unlikely to solve our problem.

@bouweandela bouweandela deleted the disable-prospector-autodetect branch September 19, 2022 11:38
@valeriupredoi
Copy link
Contributor

@bouweandela I think this would have been useful after all - in theory, this should have done the job, so it's solid implementation from you (us as a package), so I would merge this, and flag the issue at prospector's end?

@bouweandela
Copy link
Member Author

This is really not needed. If you look at the error message on Codacy carefully, like I didn't do and @zklaus did, then you'll see that they are generated by pylint and not prospector.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bear at a dinner party Something very unexpected installation Installation problem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Codacy issue Django was not configured keeps popping up in pull requests
4 participants