lint fails after fix & fmt didn't changed anything

[thelittlebug]

Describe the bug the title says it

Pants version current main d05c4370b9cae86763f7b9826c5a16ac772ebb99

OS Linux pop-os 6.0.2-76060002-generic #202210150739~1666289067~22.04~fe0ce53 SMP PREEMPT_DYNAMIC Thu O x86_64 x86_64 x86_64 GNU/Linux

Additional info this is the file that didn't pass the lint but it passes the fix & fmt without any changes

import tuhls.dashboard.settings as dashboard_settings
import utils.config as cfg
from django.conf import settings

c = cfg.ConfigDict()
apps = cfg.ConfigList(
    "django.contrib.admin",
    "django.contrib.auth",
    "django.contrib.contenttypes",
    "django.contrib.sessions",
    "tuhls.dashboard",
    "tuhls.dashboard.tests.dashboard_test",
    "tuhls.icons",
)
c = cfg.base_settings(c, "dashboardtest", apps, True)
c = cfg.template_settings(c)
c = cfg.db_sqlite_test_settings(c)
c = dashboard_settings.base(c)
c.ROOT_URLCONF = "tuhls.dashboard.urls"
c.AUTH_USER_MODEL = "dashboard_test.TestUser"
c.LOGIN_URL = "/login/"
c.LOGOUT_REDIRECT_URL = "/"
settings.configure(**c)

as you can see, isort removed the double blank line after the imports (i think that's wrong) but if i add the double blank line and run only lint it fails anyway it could be a isort bug. i have ~1k python source files and it happens only in this file

is there an easy way to change the python tool versions?

[thelittlebug]

i think i've found the way to change the isort version: https://www.pantsbuild.org/docs/reference-isort#version so i can narrow it down a little bit further

[thelittlebug]

with pants 2.14 this file gets fixed to

from django.conf import settings

import tuhls.dashboard.settings as dashboard_settings
import utils.config as cfg

c = cfg.ConfigDict()
apps = cfg.ConfigList(
    "django.contrib.admin",
    "django.contrib.auth",
    "django.contrib.contenttypes",
    "django.contrib.sessions",
    "tuhls.dashboard",
    "tuhls.dashboard.tests.dashboard_test",
    "tuhls.icons",
)
c = cfg.base_settings(c, "dashboardtest", apps, True)
c = cfg.template_settings(c)
c = cfg.db_sqlite_test_settings(c)
c = dashboard_settings.base(c)
c.ROOT_URLCONF = "tuhls.dashboard.urls"
c.AUTH_USER_MODEL = "dashboard_test.TestUser"
c.LOGIN_URL = "/login/"
c.LOGOUT_REDIRECT_URL = "/"
settings.configure(**c)

and the lint step is happy about it

[thelittlebug]

the isort version is the same in main and 2.14.

could it be that isort isn't detecting django as an external lib?

[Eric-Arellano]

This might be https://github.com/pantsbuild/pants/issues/15069. Are you running the exact same command with fmt vs. lint, i.e. the same e.g. file arguments? ./pants fmt path/to/dir and then ./pants lint path/to/dir? If those arguments of what to run are are different, then https://github.com/pantsbuild/pants/issues/15069 may be kicking in.

[benjyw]

Yeah, https://github.com/pantsbuild/pants/issues/15069 was going to be my guess. The workaround is to configure the first-party packages explicitly in isort config.

[thelittlebug]

im running ./pants fix lint ::

edit: and its running fine with 2.14

[thelittlebug]

tried every combination of known_first_party known_third_party default_section resolve_all_configs and isort version with the exact same outcome :(

[stuhood]

Are you using both black and isort, or only isort?

[thelittlebug]

both, but with this setting in pyproject.toml

[tool.isort]
profile = "black"

and it worked before, in 2.14 and 2.13 ;)

[benjyw]

Hmm, I can't reproduce this on that file so far.

[benjyw]

@thelittlebug are you able to create a standalone github repo that reproduces the problem (either at Pants 2.15.0a0, or while running from Pants sources at the SHA you mentioned)? Right now I can't reproduce it.

[gabrielhora]

Any solution to this?

We are having the same issue migrating from 2.14 to 2.15. On 2.14 running ./pants fmt lint :: had no issues, on 2.15 the fmt command does not change anything but lint fails.

./pants --no-pantsd fmt lint ::                                                                                                                                                      FTN-1328_update_pants
18:10:21.76 [INFO] Completed: Format with Black - black made no changes.
18:10:23.14 [INFO] Completed: Format with isort - isort made no changes.

✓ black made no changes.
✓ isort made no changes.
18:10:23.37 [INFO] Completed: Format with Black - black made no changes.
18:10:23.42 [INFO] Completed: Lint with Flake8 - flake8 succeeded.
Partition: ['CPython>=3.10']

18:10:23.43 [WARN] Completed: Format with isort - isort made changes.
  # list of files with changes

# some more irrelevant logs

✓ bandit succeeded.
✓ black succeeded.
✓ flake8 succeeded.
✕ isort failed.

[benjyw]

Huh, this probably isn't https://github.com/pantsbuild/pants/issues/15069 then, since that would kick in if you were not running on the same underlying set of files each time. But you're running on ::.

A small repo that reproduces this issue would be the best way to proceed with debugging this. @gabrielhora are you able to create one based on one of the problematic files? Feel free to obscure the content of the file to avoid exposing proprietary code, we just need some reliable way to reproduce the problem.

[gabrielhora]

I'm trying to come up with a test repo but no luck so far. I have no idea what might be causing this in our main repository (which is quite sizeable by now) so it's really hard to come up with a test scenario.

There doesn't seem to be anything special with the files that lint fails, they have third-party, stdlib and local imports.

[gabrielhora]

Not sure if this helpful but in my case it looks like it's not recognising the lib.redacted as a local package anymore, so apparently it's trying to move it to a third-party import block for some reason.

--- /private/var/folders/5b/1gwwv9v51hb82w76bhcxs5680000gn/T/pants-sandbox-PVkRrO/src/redacted/extractor/tests/samples.py:before       2023-03-21 20:48:37.823102
+++ /private/var/folders/5b/1gwwv9v51hb82w76bhcxs5680000gn/T/pants-sandbox-PVkRrO/src/redacted/extractor/tests/samples.py:after        2023-03-21 20:48:38.111443
@@ -6,10 +6,10 @@
 from typing import Any, Dict, List

 import yaml
+from lib.redacted.parser.main import from_bytes as parse
+from lib.redacted.parser.schemas import record_to_dict
 from ruamel.yaml import YAML

-from lib.redacted.parser.main import from_bytes as parse
-from lib.redacted.parser.schemas import record_to_dict
 from redacted.extractor.schema import Record

[gabrielhora]

So, I've added this to isort settings and that fixed the issue! Now fmt and lint are working well together and the lib.* imports are grouped with other local first party imports.

[tool.isort]
known_first_party = ["lib.*"]

It's strange that only packages in lib/ have this problem, all other first party packages are recognised correctly! Something special with that name?

[benjyw]

Huh. I guess there must be. But the disturbing part is why this was different between fix and lint.

[benjyw]

And it sounds like that doesn't happen if you take black out of the mix?

[benjyw]

There is this 3rdparty library: https://pypi.org/project/lib/

[benjyw]

But I think that's a red herring, presumably isort knows about stdlib, and looks for firstparty in your source roots, and assumes everything else is thirdparty.

[benjyw]

What does pants roots show? Is the parent of lib a source root? Is it supposed to be? I.e., are you intentionally importing from lib. redacted rather than from redacted?

[gabrielhora]

And it sounds like that doesn't happen if you take black out of the mix?

Removing black does not change anything. Without the known_first_party it still fails.

But I think that's a red herring, presumably isort knows about stdlib, and looks for firstparty in your source roots, and assumes everything else is thirdparty.

And the weird part as well is why pants 2.14 works as expected and 2.15 changes this behaviour.

What does pants roots show? Is the parent of lib a source root? Is it supposed to be? I.e., are you intentionally importing from lib. redacted rather than from redacted?

$ ./pants roots
.
src

lib is a normal package like any other inside the src source root. Sorry, my previous output might be a bit confusing because of the repeated redacted name. The folder structure look something like this:

.
├── src
│   ├── lib
│   │   ├── subpackage_1
│   │   └── subpackage_2
│   ├── package1
│   │   ├── another_subpackage_1
│   │   └── another_subpackage_2
│   ├── package2
│   └── ...
├── pants
└── pants.toml

[benjyw]

Yeah that 2.14->2.15 thing is a good clue. Any kind of reproduction repo you can share would be fantastic.

[gabrielhora]

Hey @benjyw I think I finally got a reproducible repository https://github.com/gabrielhora/pants_17403.

It looks like it has to do with resolves, if I remove the multiple resolves and the parametrize calls in the BUILD files then it all works. So maybe I have something mis-configured? This seems to work on 2.14.0 but fails on 2.15.0.

[benjyw]

Thanks, this is golden. I can reproduce, so will dive deeper.

[benjyw]

Looks like fmt runs in two partitions, one for the lib/ files and one for the project/ files. Not sure why yet, but presumably related to the two resolves.

Whereas lint runs over all files in one partition.

So when fmt runs on src/project/main.py it doesn't have lib/ in the sandbox, and so doesn't see it as first-party, whereas when lint runs, it does.

Note also that in lint mode, Pants lists each file under lib/ twice as input to isort, presumably once per resolve those python_sources are parameterized over (although this doesn't seem to be harmful, it indicates that Pants doesn't know what it's doing).

So it seems like there is major confusion in Pants on how to handle resolves here. There is partitioning discrepancy between lint and fmt, and lint sees the lib files twice.

[thejcannon]

So this is ultimately an isort issue (depending on how you're looking at it) since any formatter shouldnt be changing behavior when you run it on M files, or a subset of M files.

(Not saying there isn't a Pants bug (or many) but fixing those would paper over the underling issue)

[thejcannon]

Ah OK, this makes sense if you look at the world from Pants' point of view.

First let's look at fmt:

In fmt's eyes, we have a tool to run and we have files. We don't care about pretty much any metadata associated with the file. We don't care about the file's dependencies. We don't care about the file's resolves. Make 1000 resolves for the file, it's still the same file :smile: So when we look at what to format, we look at files (albeit we didn't dedup when we went from addresses -> files, whoops!)

Ok, now for lint:

lint operates on targets. You can lint things that aren't files (I lint python_requirements at $work to check their licenses). So when lint needs to divvy things up, it does so by address. Well now we've diverged from fmt, because 1 file now has multiple addresses.

...and :boom: we've run isort on different batches and the nasty nasty little bug that persistently plagues us appears.

---

So, we definitely should de-dup in fmt. I'll open a new ticket for that. As for de-duping in lint.... yeah maybe? I'll keep this open as I think about it.

Either way, thanks for the report and reproduction. This was very helpful!

[thejcannon]

FWIW I'm going to close as duplicate of https://github.com/pantsbuild/pants/issues/15069 Be sure to track that one!

[thejcannon]

Re-opening because, oddly enough, we can paper over this, lol