https://pantsbuild.org/ logo
Join Slack
Powered by
Question re the interpreter selection logic for ru...
# development
h
Question re the interpreter selection logic for running Black:
cc @hundreds-father-404 who is the expert here, I believe
From the explanation in the comment, it looks like we must use Python 3.8 if any code requires 3.8 or higher (e.g., has a >=3.8 constraint)
And we do this by merging the constraints, so if any constraint has that requirement, then now the entire merged constraint set does?
So the requires_python38_or_newer does the right thing?
The issue is that the constraints may not merge!
You may happen to have code with conflicting requirements in that set. Black will run on them just fine, but we won't let it do so because of this merge
So I think what we want is to check each constraint directly, without merging them. Does that sound right?
h
but we run Black in a batch. Also running w/ a newer version can understand older interpreter constraints still What concrete issue are you facing?
h
What I described
we cannot merge constraints at that point, just to figure out if any of them require 3.8+, because they may not merge
E.g., if we parameterize tests on interpreter constraints and then 
./pants fmt ::
then we try and merge over the 
::
which fails
I don't think we group Black by IC?
AFAICT that code is just an affordance to not require 3.8+ only for black, in repos that otherwise don't need it
We would still be correct if we scrapped that check and just required 3.8+ to run Black
I think?
Not that I'm suggesting we do this...
It is a nice affordance
I'm just suggesting implementing it in a different way
That does not require merging potentially incompatible ICs
I have this working, just wanted to confirm that I'm not off the rails
h
AFAICT that code is just an affordance to not require 3.8+ only for black, in repos that otherwise don't need it
correct
We would still be correct if we scrapped that check and just required 3.8+ to run Black
Yep. But the issue is if your code is 3.10+, then we need to set the constraint to 3.10
btw this generic problem is an issue w/ 
generate-lockfiles
for tool lockfiles too, especially for tools like Flake8 and Pytest that don't have a subsystem option. We try to merge your ICs
h
Hm yeah
I pushed a fix for this to https://github.com/pantsbuild/pants/pull/15414 (the one-but-last commit) but it sounds like this isn't enough
So it's fine to run Black on 3.10 if your code has 
<3.10
constraints?
Or should we be partitioning?
That seems spurious, since this is all the same code, under a parametrized python_tests target! We don't want to run black multiple times on the same code
So probably the actual workaround is to set the tool 
interpreter_constraints
explicitly
Because in that case none of this logic runs
I'll see about making that clear in the error message
OK, pushed another commit to https://github.com/pantsbuild/pants/pull/15414 which adds the workaround to the error message
h
i like your new approach
but note lockfile generation is still broken
h
The black tool lockfile, or generally?
h
I encourage you to split out Black changes into a dedicated pull request. It is super intricate code, eg coupling of lockfile logic w/ rules.py
It's only some of our lock files: MyPy, Flake8, Pylint, Pytest, IPython, Black iirc
They try merging every targets constraints into a single value. Which often is nonsensical
h
Yeah
I'm not going to tackle that just yet
h
I'm not sure that it's safe to change the black logic without changing its lock file. They are supposed to be coupled
h
The black logic hasn't changed
It's just an error message change, and moving that merge to where it's safe to do so
I reverted my attempt to be smart here, since it wasn't that smart
the new thing is just an error message fix
Unless I'm missing something?
h
oh weird i read that code wrong somehow. mea culpa
h
Naa that's probably on me for making a change and then reverting it
👍 1
h
ahhh yeah diff commits. cool. thanks
h
But at least in the repo I'm working on, that exhibits this issue, generating lockfiles works fine even with those conflicts
Erk
but... running flake8 does the bad thing, it partitions by IC
So we run 6 times on the same code...
Oh boy
Maybe parametrizing targets with sources (python_tests in this case) is not a great idea
But how else to run tests on multiple interpreters?
Can always run 
./pants lint tests/@interpreter_constraints=py37
of course
but 
./pants lint ::
will fail, and that is a shame
Well, not fail exactly, but run flake8 6 times, and may fail
We can't force a flake8 interpreter for the repo, and maybe we should allow that, to bypass all this
I'll start a new thread for this conversation, since it's no longer related to the original topic
h
feel free to reopen. idk what the solution would look like. and there are times where you actually really do want to run w/ different ICs. (Although imo different resolves is more relevant)
h
Commented there
Open in Slack
PreviousNext
Powered by