https://pantsbuild.org/ logo
Powered by
a

ancient-france-42909

05/04/2023, 5:30 PM
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

witty-crayon-22786

05/04/2023, 6:38 PM
are the targets that own the init files parameterized as well?
a

ancient-france-42909

05/04/2023, 6:39 PM
Hm. Oh, I guess the init files need to go as test_utils?
w

witty-crayon-22786

05/04/2023, 6:39 PM
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

ancient-france-42909

05/04/2023, 6:40 PM
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

witty-crayon-22786

05/04/2023, 6:42 PM
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

ancient-france-42909

05/04/2023, 6:43 PM
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

witty-crayon-22786

05/04/2023, 6:45 PM
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

ancient-france-42909

05/04/2023, 6:46 PM
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

witty-crayon-22786

05/04/2023, 6:48 PM
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

ancient-france-42909

05/04/2023, 6:49 PM
We had problems with this in the past, it was inferring a dependency that wasn't there
w

witty-crayon-22786

05/04/2023, 6:49 PM
not sure if that will work… the interaction of 
parametrize
and 
overrides
is … interesting to reason about.
a

ancient-france-42909

05/04/2023, 6:49 PM
Let me try, but the upper bound should be 
<3.11
w

witty-crayon-22786

05/04/2023, 6:50 PM
…ah, dang. no, it will not work: https://github.com/pantsbuild/pants/issues/14535
a

ancient-france-42909

05/04/2023, 6:51 PM
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

witty-crayon-22786

05/04/2023, 6:53 PM
mm… the 
__init__.py
also has dependencies? fun.
a

ancient-france-42909

05/04/2023, 6:53 PM
Why would they need to be a subset of those deps? The intersection should exist, no?
w

witty-crayon-22786

05/04/2023, 6:55 PM
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

ancient-france-42909

05/04/2023, 6:55 PM
(I will admit our dependencies are absolutely broken, I'm surprised anything ever works in our codebase... )
w

witty-crayon-22786

05/04/2023, 6:55 PM
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

ancient-france-42909

05/04/2023, 6:55 PM
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

witty-crayon-22786

05/04/2023, 6:59 PM
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

ancient-france-42909

05/04/2023, 6:59 PM
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

witty-crayon-22786

05/04/2023, 7:00 PM
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

ancient-france-42909

05/04/2023, 7:02 PM
We definitely want to parametrise tests, otherwise how can we know whether it works? 😄
w

witty-crayon-22786

05/04/2023, 7:02 PM
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

ancient-france-42909

05/04/2023, 7:04 PM
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

witty-crayon-22786

05/04/2023, 7:04 PM
i’d let 
tailor
choose it’s default =P
a

ancient-france-42909

05/04/2023, 7:05 PM
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

witty-crayon-22786

05/04/2023, 7:07 PM
is there a reason you have glob overrides, rather than using what 
tailor
does by default?
a

ancient-france-42909

05/04/2023, 7:08 PM
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

witty-crayon-22786

05/04/2023, 7:08 PM
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

ancient-france-42909

05/04/2023, 7:09 PM
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

witty-crayon-22786

05/04/2023, 7:10 PM
that’s correct… so to use 
__defaults__
, you’d have to remove the explicit arguments
a

ancient-france-42909

05/04/2023, 7:10 PM
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

witty-crayon-22786

05/04/2023, 7:15 PM
to kill the copy-pasta forever, 
__defaults__
and then nuking that field might be worth it. but yea, up to you.
a

ancient-france-42909

05/04/2023, 7:16 PM
The tests aren't always in the same directory, relative to the 
BUILD
w

witty-crayon-22786

05/04/2023, 7:16 PM
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

ancient-france-42909

05/04/2023, 7:18 PM
I don't, but I don't think we've ever used 
tailor
so I have no idea what that generates
w

witty-crayon-22786

05/04/2023, 7:19 PM
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

ancient-france-42909

05/04/2023, 7:21 PM
I ran 
tailor
on that folder, it was useless, heh
it just added a 
python_sources()
at the end of the 
BUILD
files
w

witty-crayon-22786

05/04/2023, 7:22 PM
yep: that target likely owns a file that wasn’t being matched before
a

ancient-france-42909

05/04/2023, 7:22 PM
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

witty-crayon-22786

05/04/2023, 7:23 PM
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

ancient-france-42909

05/04/2023, 7:24 PM
There are no 
sources
for any of the things it added.
w

witty-crayon-22786

05/04/2023, 7:24 PM
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

ancient-france-42909

05/04/2023, 7:24 PM
We have things like 
sources=["filters/**/*.py"]
And the default source glob won't add 
__init__.py
files?
w

witty-crayon-22786

05/04/2023, 7:25 PM
correct
a

ancient-france-42909

05/04/2023, 7:25 PM
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

witty-crayon-22786

05/04/2023, 7:28 PM
it doesn’t impact the number of targets that are created.
there will still be a target per file under the hood.
a

ancient-france-42909

05/04/2023, 7:29 PM
Fair enough, so we need to have less files to make it faster 🙂
w

witty-crayon-22786

05/04/2023, 7:29 PM
when you say “resolve deps”, do you mean parsing the dependencies for inference? or actual thirdparty resolves with pip/PEX?
a

ancient-france-42909

05/04/2023, 7:31 PM
Resolve transitive targets
, not sure which one that is.
w

witty-crayon-22786

05/04/2023, 7:31 PM
the former. interesting.
a

ancient-france-42909

05/04/2023, 7:32 PM
We removed a lot of useless deps, dep cycles and such lately, it's much faster now, only takes ~40 seconds.
w

witty-crayon-22786

05/04/2023, 7:32 PM
a

ancient-france-42909

05/04/2023, 7:33 PM
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
Powered by