Linting build not working

[apalha]

The Build workflow fails in this line:

https://github.com/NLeSC/python-template/blob/807f751c2ce759e6e3464f714f2ef69929858f7e/%7B%7Bcookiecutter.directory_name%7D%7D/.github/workflows/build.yml#L62

The error is the following:

Run prospector
  prospector
  shell: /usr/bin/bash -e {0}
  env:
    pythonLocation: /opt/hostedtoolcache/Python/3.9.16/x64
    LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.9.16/x64/lib

Cannot run tool pyroma as support was not installed.
Please install by running 'pip install prospector[with_pyroma]'

Error: Process completed with exit code [2](https://github.com/EDG-Acoustics/edg-acoustics/actions/runs/3882655432/jobs/6623058120#step:7:2).

I have forced installing prospector with pyroma, adding a line python3 -m pip install prospector[with_pyroma], forced the reinstall with --ignore-installed, and confirmed that it is installing it (seems so):

INFO: pip is looking at multiple versions of pyroma to determine which version is compatible with other requirements. This could take a while.
Collecting pyroma>=2.4
  Downloading pyroma-4.0-py2.py3-none-any.whl (28 kB)

I still get the error. Did someone face the same problem or does anyone know what could be the issue?

Thanks!

[LourensVeen]

Prospector tries to do from pyroma import projectdata, ratings, but unfortunately swallows any exception raised and replaces it with the warning you're seeing there. Perhaps you could put a simple Python script in the workflow that runs the above statement, and see what it says?

[apalha]

Hi @LourensVeen , thanks for looking into this.

I added a line to run that:

Run python3 -c "from pyroma import projectdata, ratings"
  python3 -c "from pyroma import projectdata, ratings"
  shell: /usr/bin/bash -e {0}
  env:
    pythonLocation: /opt/hostedtoolcache/Python/3.9.16/x64
    LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.9.16/x64/lib

And I see no error. Do you think this error is related to something else that is not the installation of pyroma?

[LourensVeen]

Ah no, this should do the trick, and it does work. So that's weird. The error seems to come from https://github.com/PyCQA/prospector/blob/v1.8.3/prospector/tools/__init__.py, which tries to import https://github.com/PyCQA/prospector/blob/v1.8.3/prospector/tools/pyroma/__init__.py, which runs that statement and presumably raises an ImportError, except it seems to work if you run it separately.

What does python -c "import prospector.tools.pyroma" do?

[apalha]

Seems like there is no error:

Run python3 -c "import prospector.tools.pyroma"
  python3 -c "import prospector.tools.pyroma"
  shell: /usr/bin/bash -e {0}
  env:
    pythonLocation: /opt/hostedtoolcache/Python/[3](https://github.com/EDG-Acoustics/edg-acoustics/actions/runs/3883412584/jobs/6624690574#step:7:3).9.16/x6[4](https://github.com/EDG-Acoustics/edg-acoustics/actions/runs/3883412584/jobs/6624690574#step:7:4)
    LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.9.1[6](https://github.com/EDG-Acoustics/edg-acoustics/actions/runs/3883412584/jobs/6624690574#step:7:6)/x64/lib`

[LourensVeen]

Ah, that's weird. Maybe try from the other side with from prospector.run import main ; main()? I would expect that to not work, and the problem to be somewhere in between, but who knows...

[egpbos]

DIANNA had this issue as well, perhaps it is the same: https://github.com/dianna-ai/dianna/issues/423

[LourensVeen]

Interesting. It seems that the Prospector issue linked from the DIANNA issue only fixes the problem that pyroma doesn't install automatically, but user volans also mentions that it doesn't run correctly, and that's what we're seeing here as well.

So I did some more investigating and found the problem, only to discover that it was found by someone else four days ago, and that a fix had been in the repo for an hour already!

https://github.com/PyCQA/prospector/pull/574

So it should work again from 1.8.4 onwards. Meanwhile, the solution is to limit the prospector version to <1.8.0; I cannot reproduce this on 1.7.7 at least.

[apalha]

I tried with version 1.7.7 and now I get a different error.

prospector
  shell: /usr/bin/bash -e {0}
  env:
    pythonLocation: /opt/hostedtoolcache/Python/[3](https://github.com/EDG-Acoustics/edg-acoustics/actions/runs/3892052701/jobs/6643012210#step:7:3).9.16/x6[4](https://github.com/EDG-Acoustics/edg-acoustics/actions/runs/3892052701/jobs/6643012210#step:7:4)
    LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.9.16/x64/lib
Messages
========

.
  Line: None
    pylint: failure / Tool pylint failed to run (exception was raised, re-run prospector with -X to see the stacktrace)

setup.py
  Line: None
    pyroma: PYRUNKNOWN / You should specify what Python versions you support with the 'requires-python'/'python_requires' metadata.

Check Information
=================
         Started: 2023-01-11 10:48:[5](https://github.com/EDG-Acoustics/edg-acoustics/actions/runs/3892052701/jobs/6643012210#step:7:5)8.[6](https://github.com/EDG-Acoustics/edg-acoustics/actions/runs/3892052701/jobs/6643012210#step:7:6)65604
        Finished: 2023-01-11 10:4[8](https://github.com/EDG-Acoustics/edg-acoustics/actions/runs/3892052701/jobs/6643012210#step:7:9):5[9](https://github.com/EDG-Acoustics/edg-acoustics/actions/runs/3892052701/jobs/6643012210#step:7:10).[16](https://github.com/EDG-Acoustics/edg-acoustics/actions/runs/3892052701/jobs/6643012210#step:7:17)2639
      Time Taken: 0.50 seconds
       Formatter: grouped
        Profiles: .prospector.yml, no_doc_warnings, strictness_medium, strictness_high, strictness_veryhigh, no_member_warnings
      Strictness: from profile
  Libraries Used: 
       Tools Run: dodgy, mccabe, profile-validator, pycodestyle, pyflakes, pylint, pyroma
  Messages Found: 2
 External Config: pylint: /home/runner/work/edg-acoustics/edg-acoustics/pyproject.toml

Error: Process completed with exit code 1.

So I ran it with prospector -X and got this:

Run prospector -X
  prospector -X
  shell: /usr/bin/bash -e {0}
  env:
    pythonLocation: /opt/hostedtoolcache/Python/3.9.16/x64
    LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.9.16/x64/lib
Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/prospector/run.py", line 94, in execute
    messages += tool.run(found_files)
  File "/opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/prospector/tools/pylint/__init__.py", line [2](https://github.com/EDG-Acoustics/edg-acoustics/actions/runs/3892076725/jobs/6643067461#step:7:2)5[3](https://github.com/EDG-Acoustics/edg-acoustics/actions/runs/3892076725/jobs/6643067461#step:7:3), in run
    self._linter.check(self._args)
  File "/opt/hostedtoolcache/Python/3.9.16/x6[4](https://github.com/EDG-Acoustics/edg-acoustics/actions/runs/3892076725/jobs/6643067461#step:7:4)/lib/python3.9/site-packages/pylint/lint/pylinter.py", line [6](https://github.com/EDG-Acoustics/edg-acoustics/actions/runs/3892076725/jobs/6643067461#step:7:6)96, in check
    ast_per_fileitem = self._get_asts(fileitems, data)
  File "/opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/pylint/lint/pylinter.py", line [7](https://github.com/EDG-Acoustics/edg-acoustics/actions/runs/3892076725/jobs/6643067461#step:7:8)07, in _get_asts
    for fileitem in fileitems:
  File "/opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/pylint/lint/pylinter.py", line 874, in _iterate_file_descrs
    for descr in self._expand_files(files_or_modules).values():
  File "/opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/prospector/tools/pylint/linter.py", line 24, in _expand_files
    if self._files.check_module(module["path"]):
TypeError: string indices must be integers

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.9.16/x64/bin/prospector", line [8](https://github.com/EDG-Acoustics/edg-acoustics/actions/runs/3892076725/jobs/6643067461#step:7:9), in <module>
    sys.exit(main())
  File "/opt/hostedtoolcache/Python/3.[9](https://github.com/EDG-Acoustics/edg-acoustics/actions/runs/3892076725/jobs/6643067461#step:7:10).16/x64/lib/python3.9/site-packages/prospector/run.py", line 202, in main
    prospector.execute()
  File "/opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/prospector/run.py", line [11](https://github.com/EDG-Acoustics/edg-acoustics/actions/runs/3892076725/jobs/6643067461#step:7:12)2, in execute
    raise FatalProspectorException from ex
TypeError: __init__() missing 1 required positional argument: 'message'
Error: Process completed with exit code 1.

This seems to be deep inside prospector right? Any ideas?

[LourensVeen]

Haha, van de regen in de drup :).

Seems like this has been fixed in https://github.com/PyCQA/prospector/pull/530, so if you upgrade to Prospector 1.8.0 or later, then it should work again!

[LourensVeen]

(https://github.com/PyCQA/prospector/issues/539 says that downgrading pylint to 2.15.6 also works.)

Maybe we should just get rid of prospector? I just call flake8 and sometimes pydocstyle from tox and call it a day... On the other hand, it seems that this 1.8 release is just botched, so maybe in the coming weeks they'll fix the teething issues and it'll all be fine again.

[apalha]

@LourensVeen and @egpbos ,

Following up on Lourens proposal. If I were to replace prospector with flake8 how should I go about it?

1. Change these lines https://github.com/NLeSC/python-template/blob/807f751c2ce759e6e3464f714f2ef69929858f7e/%7B%7Bcookiecutter.directory_name%7D%7D/.github/workflows/build.yml#L32-L35

to

- name: Upgrade pip and install dependencies
        run: |
          python3 -m pip install --upgrade pip setuptools
          python3 -m pip install .[dev,publishing]
          python3 -m pip install flake8

2. Change this line https://github.com/NLeSC/python-template/blob/807f751c2ce759e6e3464f714f2ef69929858f7e/%7B%7Bcookiecutter.directory_name%7D%7D/.github/workflows/build.yml#L62

to

run: flake8

3. Change this line https://github.com/NLeSC/python-template/blob/807f751c2ce759e6e3464f714f2ef69929858f7e/%7B%7Bcookiecutter.directory_name%7D%7D/setup.cfg#L52

to

flake8

4. Change these lines https://github.com/NLeSC/python-template/blob/807f751c2ce759e6e3464f714f2ef69929858f7e/%7B%7Bcookiecutter.directory_name%7D%7D/.githooks/pre-commit#L8-L9

to

# quietly run flake8
flake8 {{ cookiecutter.package_name }} 1>/dev/null

5. Remove this file https://github.com/NLeSC/python-template/blob/807f751c2ce759e6e3464f714f2ef69929858f7e/%7B%7Bcookiecutter.directory_name%7D%7D/.prospector.ym

Do we need to replace it with an equivalent for flake8?

Something else is needed?

[egpbos]

I think I would be against replacing prospector. It's true that it sometimes bugs a bit, but that's probably because it combines a lot of tools. If we were to combine all those tools ourselves in our custom workflow we would carry the same maintenance burden. I personally think they are all useful, although surely this differs from project to project.

[egpbos]

Note btw that there was a new prospector release 1.8.4 yesterday. The issues that were fixed for 1.8.4 listed here include both a pylint failure bug and a pyroma include thingy... Maybe good to try out.

[egpbos]

I'm trying it out in the DIANNA repo right now, see the mention above.

[egpbos]

The linter installs and runs without issues there, so try using 1.8.4 :)

[apalha]

@egpbos it is working now but I get a similar error to the one in DIANNA regarding imports. it gives an error on importing pytest. Pytest is installed and if I run the python file it complains about, there is no error with importing pytest.

This is from DIANNA:

dianna/methods/lime.py
  Line: 2
    pylint: import-error / Unable to import 'lime.lime_image'
  Line: 3
    pylint: import-error / Unable to import 'lime.lime_text'

[apalha]

@LourensVeen and @egpbos I have it working now on git (with v1.8.4). For some reason it gives the import error on my laptop (macOS).

Thanks a lot for your help!

[egpbos]

Hm very weird! I hope the import error stays away then or that it can be fixed from the DIANNA side. Let's wait and see.

[LourensVeen]

Yay! Yes, since both issues were fixed already in prospector git, it makes sense that they got included in 1.8.4. Looking at the change log it looks like 1.8.0 was released in a quite broken state, and there's been a flurry of new releases with fixes since. That's good at least, but maybe it needs a better test suite or something...

[apalha]

I have a theoretical question. Does it make sense to always force a specific version of dependencies? Or at least use <=. I say this because something may work fine on the latest version in 2021 and then in 2022 someone else uses the code and it breaks in all kinds of weird ways. In the end it is a new version of one dependency that was broken or incompatible? Just like in this case. If every year or six months we look into updating the dependencies then we know the issue is with dependencies. If a power user wants/needs to use the cutting edge version of a dependency, then they are on their own (but they also know what changed).

[jiskattema]

@apalha that's the problem semantic versioning is supposed to solve. Major version number increases (can be / are allowed to be) incompatible ; minor version number increases (should be) backwards compatible. A point release should be only bug fixes etc. Then it's up to the user if they want latest or not. For security reasons, it makes sense to always use the latest release, but for some other usecase stability (and no changes) is more important.

[apalha]

@jiskattema yes, semantic versioning informs the developper. A user that uses a code does not know if the code was written for version 1.x.y or for version 2.z.w. And at installation time there is no control over that, unless we force <2.0.0. Still, if working version 1.x.y was updated to version 1.x+1.0 there is always the possibility of introducing a bug, which was the case here. This then makes the code that requires that dependency unusable until the cause of the issue is found.

I must admit I am very conservative regarding using latest releases by default, unless there are specific reasons/requirements to do so. Especially when the required ecosystem becomes more complex the possibility of unexpected incompatibilities grows exponentially.

[egpbos]

If you have a couple of hours, take a look at this ;) https://iscinumpy.dev/post/bound-version-constraints/

Tl;dr: https://iscinumpy.dev/post/bound-version-constraints/#tldr

We also discussed it (incompletely) in Teams at some point, you can probably find it if you search for the link.

[egpbos]

It also has a tl;dr to the tl;dr:

Libraries/packages should be setting a floor, and if necessary excluding known buggy versions, but otherwise don’t cap the maximum version as you can’t predict future compatibility

I would agree this is a good default policy, except I would make it even shorter:

~Libraries/packages should be setting a floor, and~ if necessary exclud~ing~e known buggy versions, but otherwise don’t cap the maximum version as you can’t predict future compatibility

You can floor if users complain or if you anticipate your package's use on old systems, but in typical usecases (installing things in fresh environments on recent Python versions) you don't really need to because you'll get all the latest versions by default.

[egpbos]

Having said all that, you are not alone in your "conservative" view (see the Teams discussion :D) and it can be very valid in certain cases (see the article), but just be aware that there are good arguments for "liberal" dependency versioning and that this in my experience is also what a large part of the Python community practices.

[apalha]

I scanned the article. A strong argument is interaction with other libraries, which you do not control. You need library A with R.A requirements, but then you need B with R.B requirements, with strict (conservative) versions the compatible intersection can (and will) be zero.

[LourensVeen]

I'm not super-strict with this, as it's all broken anyway, but I can see both sides of the equation. Numpy for example releases fairly frequently and doesn't guarantee a lot of future stability, so setting an upper bound that is guaranteed to work means you need to re-test and re-release often. Wait too long and your software won't install anymore, even though it'll probably still work. That's been an issue with MUSCLE3.

For YAtiML, I heavily depend on ruamel.yaml and that is a mess, active development, no defined API, no compatibility guarantees, and broken releases fairly regularly. I have a script that tests against all releases of the last couple of years, and then put in a version constraint with lower and upper bounds and excludes for known-broken versions. I've had issues with people installing some other software that required a recent version that I hadn't tested against and getting version conflicts, but I don't dare to release the constraints either. The real solution is to finally invent NAML, but well, when I find a round tuit.

Otherwise, I'm also often sloppy and don't set a version constraint at all. If the dependency is some random Python package that some dude (f/m) made years ago and hasn't touched since, then it'll continue to work anyway...