https://pantsbuild.org/ logo
Join Slack
Powered by
We're going through a very painful exercise of upg...
# general
a
We're going through a very painful exercise of upgrading our apps to python 3.10. As part of this, we go through libraries and we mark them as compatible by adding
Copy code
interpreter_constraints=[
        "CPython~=3.7.4",
        "CPython~=3.10.9",
    ]
to 
python_sources
and
Copy code
interpreter_constraints=parametrize(
        py37=["~=3.7.4"],
        py310=["~=3.10.9"],
    ),
to our 
python_tests
. The idea being, we mark libs as compatible, then run tests on both. So this works sort of okay, until this interacts with init file inference (here). When this has the default value, we get:
Copy code
pants.engine.target.InvalidFieldException: The target filters/src/messages/tests/unit/templates/test_helpers.py:../../../test@interpreter_constraints=py37 has the `interpreter_constraints` ('~=3.7.4',), which are not a subset of the `interpreter_constraints` of some of its dependencies:

  * ('~=3.10.9',): filters/src/messages/tests/unit/templates/__init__.py:../../../test@interpreter_constraints=py310

To fix this, you should likely adjust filters/src/messages/tests/unit/templates/test_helpers.py:../../../test@interpreter_constraints=py37's `interpreter_constraints` to match the narrowest range in the above list.
Aside turning that off, what can we do?
My gut feeling is that this is a bug, but I just wanted to confirm that I'm not being stupid(er than usually)
w
are the targets that own the init files parameterized as well?
a
Hm. Oh, I guess the init files need to go as test_utils?
w
i don’t think so… i think that’s only 
conftest.py
but, basically: i think that you’d need to parameterize the targets which own the init files in parent directories.
which, if they have neighboring files (which can’t be made compatible), might mean splitting out a separate target to own the init file
a
Copy code
python_tests(
    name="test",
    sources=["tests/**/*.py", "!tests/**/conftest.py"],
    dependencies=[
        ":lib",
        ":test-utils",
        ":test-files",
        "common/testing/src:lib",
        "filters/src/core:lib",
        "filters/src/emails:lib",
        "defender:lib",
        "requirements/python#testfixtures",
        "requirements/python#html2text",
        "requirements/python#freezegun",
        "requirements/python#beautifulsoup4",
    ],
    tags=["mono"],
    interpreter_constraints=parametrize(
        py37=["~=3.7.4"],
        py310=["~=3.10.9"],
    ),
)
This is the tests target, and it includes both the 
__init__.py
and the test, in sources
:lib
is the actual library, the one that we didn't parametrise, 
test-files
are just some resources (yamls and such), 
test-utils
are 
test_utils
with `conftest.py`s and the rest are just other libs in our code
w
hm. that’s odd… inference should not be picking up the incompatible copy i don’t think.
(or, at least: i know that it doesn’t for multiple resolves…)
what does 
./pants list filters/src/messages/tests/unit/templates/__init__.py
report?
a
Copy code
filters/src/messages/tests/unit/templates/__init__.py:../../../test@interpreter_constraints=py37
filters/src/messages/tests/unit/templates/__init__.py:../../../test@interpreter_constraints=py310
The output for test_helpers has the constraints reversed, I doubt that matters, but yeah:
Copy code
filters/src/messages/tests/unit/templates/test_helpers.py:../../../test@interpreter_constraints=py310
filters/src/messages/tests/unit/templates/test_helpers.py:../../../test@interpreter_constraints=py37
w
yuck. it looks like it’s probably inferring dependencies on both copies.
so, yea… that’s a bug.
one thing that i think that you can do is override the interpreter_constraints for 
__init__.py
to be wider?
a
How?
Or, rather, how wide and how to do that for one file?
If we're going to do something (except turning inference off, I'm not really a huge fan anyway), we're gonna remove stuff from 
__init__.py
(why is it so hard to write that in slack :P)
w
Copy code
python_tests(
    name="test",
    ...,
    interpreter_constraints=parametrize(
        py37=["~=3.7.4"],
        py310=["~=3.10.9"],
    ),
    overrides={
      '__init__.py': dict(
        interpreter_constraints=[">=3.7.4,<=3.10.9"],
      )
    }
)
a
We had problems with this in the past, it was inferring a dependency that wasn't there
w
not sure if that will work… the interaction of 
parametrize
and 
overrides
is … interesting to reason about.
a
Let me try, but the upper bound should be 
<3.11
w
…ah, dang. no, it will not work: https://github.com/pantsbuild/pants/issues/14535
a
Well, that gave me a much longer error message 🙂
Which, btw, I think is wrong, unless I'm blind
Copy code
pants.engine.target.InvalidFieldException: The target filters/src/messages/tests/unit/templates/__init__.py:../../../test@interpreter_constraints=py37 has the `interpreter_constraints` ('>=3.7.4,<3.11',), which are not a subset of the `interpreter_constraints` of some of its dependencies:

  * ('CPython~=3.7.4', 'CPython~=3.10.9'): filters/src/messages/tests/conftest.py:../test_utils
  * ('CPython~=3.7.4', 'CPython~=3.10.9'): filters/src/messages/tests/unit/conftest.py:../../test_utils
  * ('~=3.7.4',): filters/src/messages/tests/unit/templates/test_defender_compliance_alert.py:../../../test@interpreter_constraints=py37
  * ('~=3.7.4',): filters/src/messages/tests/unit/templates/test_helpers.py:../../../test@interpreter_constraints=py37
  * ('~=3.7.4',): filters/src/messages/tests/unit/templates/test_html_templates.py:../../../test@interpreter_constraints=py37
  * ('~=3.7.4',): filters/src/messages/tests/unit/templates/test_plaintext_templates.py:../../../test@interpreter_constraints=py37
  * ('~=3.7.4',): filters/src/messages/tests/unit/templates/test_rtf_templates.py:../../../test@interpreter_constraints=py37
  * ('~=3.7.4',): filters/src/messages/tests/unit/test_attachment_hash_denylisted.py:../../test@interpreter_constraints=py37
  * ('~=3.7.4',): filters/src/messages/tests/unit/test_attachment_urls.py:../../test@interpreter_constraints=py37
  * ('~=3.7.4',): filters/src/messages/tests/unit/test_content_generation.py:../../test@interpreter_constraints=py37
  * ('~=3.7.4',): filters/src/messages/tests/unit/test_demo_flags_output.py:../../test@interpreter_constraints=py37
  * ('~=3.7.4',): filters/src/messages/tests/unit/test_element_list.py:../../test@interpreter_constraints=py37
  * ('~=3.7.4',): filters/src/messages/tests/unit/test_malicious_attachment_urls.py:../../test@interpreter_constraints=py37
  * ('~=3.7.4',): filters/src/messages/tests/unit/test_message_elements.py:../../test@interpreter_constraints=py37
  * ('~=3.7.4',): filters/src/messages/tests/unit/test_simple_message_element.py:../../test@interpreter_constraints=py37
  * ('~=3.7.4',): filters/src/messages/tests/unit/test_threat_intelligence.py:../../test@interpreter_constraints=py37
  * ('~=3.7.4',): filters/src/messages/tests/unit/test_warning_item_ordering.py:../../test@interpreter_constraints=py37
Ah. A subset.
w
mm… the 
__init__.py
also has dependencies? fun.
a
Why would they need to be a subset of those deps? The intersection should exist, no?
w
so… actually, try:
Copy code
python_tests(
    name="test",
    ...,
    interpreter_constraints=parametrize(
        py37=["~=3.7.4"],
        py310=["~=3.10.9"],
    ),
    overrides={
      '__init__.py': dict(
        interpreter_constraints=["~=3.7.4","~=3.10.9"],
      )
    }
)
a
(I will admit our dependencies are absolutely broken, I'm surprised anything ever works in our codebase... )
w
but, the reason they need to be a subset is that you can’t claim to be compatible with everything up to 3.11 if some of your deps aren’t.
a
Copy code
pants.engine.target.InvalidFieldException: The target filters/src/messages/tests/unit/templates/__init__.py:../../../test@interpreter_constraints=py37 has the `interpreter_constraints` ('~=3.7.4', '~=3.10.9'), which are not a subset of the `interpreter_constraints` of some of its dependencies:

  * ('~=3.7.4',): filters/src/messages/tests/unit/templates/test_defender_compliance_alert.py:../../../test@interpreter_constraints=py37
  * ('~=3.7.4',): filters/src/messages/tests/unit/templates/test_helpers.py:../../../test@interpreter_constraints=py37
  * ('~=3.7.4',): filters/src/messages/tests/unit/templates/test_html_templates.py:../../../test@interpreter_constraints=py37
  * ('~=3.7.4',): filters/src/messages/tests/unit/templates/test_plaintext_templates.py:../../../test@interpreter_constraints=py37
  * ('~=3.7.4',): filters/src/messages/tests/unit/templates/test_rtf_templates.py:../../../test@interpreter_constraints=py37
  * ('~=3.7.4',): filters/src/messages/tests/unit/test_attachment_hash_denylisted.py:../../test@interpreter_constraints=py37
  * ('~=3.7.4',): filters/src/messages/tests/unit/test_attachment_urls.py:../../test@interpreter_constraints=py37
  * ('~=3.7.4',): filters/src/messages/tests/unit/test_content_generation.py:../../test@interpreter_constraints=py37
  * ('~=3.7.4',): filters/src/messages/tests/unit/test_demo_flags_output.py:../../test@interpreter_constraints=py37
  * ('~=3.7.4',): filters/src/messages/tests/unit/test_element_list.py:../../test@interpreter_constraints=py37
  * ('~=3.7.4',): filters/src/messages/tests/unit/test_malicious_attachment_urls.py:../../test@interpreter_constraints=py37
  * ('~=3.7.4',): filters/src/messages/tests/unit/test_message_elements.py:../../test@interpreter_constraints=py37
  * ('~=3.7.4',): filters/src/messages/tests/unit/test_simple_message_element.py:../../test@interpreter_constraints=py37
  * ('~=3.7.4',): filters/src/messages/tests/unit/test_threat_intelligence.py:../../test@interpreter_constraints=py37
  * ('~=3.7.4',): filters/src/messages/tests/unit/test_warning_item_ordering.py:../../test@interpreter_constraints=py37
I've read those 5 times now, they all say 
~=3.7.4
😞
ah, SUBSET, again.
I keep thinking it should be intersection
w
so, the general problem here (which is what triggered this thread in the first place) is that dependency inference doesn’t disambiguate based on interpreter constraints: it will just give you deps on both parametrized copies. we have special handling for the 
resolve
field, but not for interpreter_constraints.
i’ll file an issue about that.
but.
a
I think I'll do this, I will fix this problem by moving crap out of 
__init__.py
, then I'll try to get a minimal reproducible example
but the example will be over the weekend or maybe even two weeks from now 😞
w
i think that there should be a solution possible by directly using the fact that 
interpreter_constraints
supports sets without parametrization
having done this type of migration before using ICs, you’d use: • converted libraries: 
interpreter_constraints=[">=3.7.4,<=3.10.9"]
• converted tests: … can either parametrize, or use one or the other version
a
We definitely want to parametrise tests, otherwise how can we know whether it works? 😄
w
but i think that you’ll need to split your 
__init__.py
files out of your test targets, regardless =/
…oh. in fact, i think that 
__init__.py
files are not included in 
python_tests
by default anyway… i think that your globs have re-included them.
so if you exclude them from those globs and then re-run 
tailor
, it should fix it for you.
a
As for changing the interpreter constraints for all our libraries, that's not really possible, we've migrated probably 100 of them, it'll be a major pain to change it.
Should I include it in test-utils?
w
i’d let 
tailor
choose it’s default =P
a
Excluding it seems to work...
I guess there's no way, using 
__defaults__
, to do this for every 
python_tests
I'll have to find the AST parsing/converting to python script I used to exclude all the conftests, I guess, heh
w
is there a reason you have glob overrides, rather than using what 
tailor
does by default?
a
We started with pants a long time ago and people just, probably, copy paste from other 
BUILD
files when they add a new lib/app
w
I guess there’s no way, using 
__defaults__
, to do this for every 
python_tests
hm… there might be? why don’t you think that 
__defaults__
will work for this?
a
Because I thought this just provides defaults for the keyword args to the targets, if you specify some value for that, it'd be overriden.
w
that’s correct… so to use 
__defaults__
, you’d have to remove the explicit arguments
a
Yeah, that's also gonna be painful 🙂
our directory structure is also a complete mess, heh
I guess we can just do it by hand, when we have this issue.
We have to be done with the upgrade by the EOL date of 3.7, so it's not like people will need to remember it for very long 🙂
w
to kill the copy-pasta forever, 
__defaults__
and then nuking that field might be worth it. but yea, up to you.
a
The tests aren't always in the same directory, relative to the 
BUILD
w
mm.
i hope you don’t mind: i’m going to use this thread as a case-study for why 111 / `tailor`’s defaults are more scalable
a
I don't, but I don't think we've ever used 
tailor
so I have no idea what that generates
w
yea, all good. history is a thing.
(it generates 1 of each type of target per directory, which matches the default globs of `python_sources`/`python_tests` etc, so globs don’t need to be specified)
a
I ran 
tailor
on that folder, it was useless, heh
it just added a 
python_sources()
at the end of the 
BUILD
files
w
yep: that target likely owns a file that wasn’t being matched before
a
Oh, it added new 
BUILD
files
Copy code
python_test_utils(
    name="test_utils",
)

python_tests(
    name="tests",
)
Not very useful either 😞
I'm not sure how this is helpful, first people need to make sure they run this command before they commit, but ALSO, they have to figure out what's missing?
w
the advantage of 
tailor
is that you never need to manually declare targets: they exist only as places to add metadata like extra dependencies, interpreter constraints, etc
it has 
--check
mode
a
There are no 
sources
for any of the things it added.
w
I’m not sure how this is helpful, first people need to make sure they run this command before they commit, but ALSO, they have to figure out what’s missing?
to be clear: people already need to figure out what files don’t have owning targets: how are you doing that now?
There are no 
sources
for any of the things it added.
it’s using the default source globs for the target
take a look at the difference between 
./pants list directory:
before and after: most likely there was an unowned file in there.
a
We have things like 
sources=["filters/**/*.py"]
And the default source glob won't add 
__init__.py
files?
w
correct
a
I hope just for tests, because otherwise that'd be... weird 🙂
And, no, it didn't add unowned files (it did add some, but specifically excluded them), most of the new ones are just 
BUILD
files per directory that we have from above.
And we already have a problem with many pants commands taking minutes to resolve deps, I doubt this will make things better.
w
it doesn’t impact the number of targets that are created.
there will still be a target per file under the hood.
a
Fair enough, so we need to have less files to make it faster 🙂
w
when you say “resolve deps”, do you mean parsing the dependencies for inference? or actual thirdparty resolves with pip/PEX?
a
Resolve transitive targets
, not sure which one that is.
w
the former. interesting.
a
We removed a lot of useless deps, dep cycles and such lately, it's much faster now, only takes ~40 seconds.
w
a
I think I might've seen that issue before. 🙂
Anyway, we do have a plan to get rid of the dep cycles (as much as we can), refactor stupid deps (we depend on huge libs just for some enum all over the place), and also try to get lockfiles working in the next quarter (or, rather, we might try to do it, depending on how persuasive I can be), so I'm hoping this won't be a huge issue soon-ish.
Anyway, I'm gonna go to bed soon. Thanks for the help, excluding 
__init__.py
did fix the problem.
👋 1
2 Views
Open in Slack
PreviousNext
Powered by