https://pantsbuild.org/ logo
Join Slack
Powered by
When I updated to 2.17, I disabled `[python-infer]...
# general
p
When I updated to 2.17, I disabled 
[python-infer].use_rust_parser
because I was getting a bunch errors I couldn't make sense of. Now I'm updating to 2.18, so I want to re-enable that. Is there additional debug output I can get from the rust parser? I've got ALOT of error messages like this:
Copy code
The target st2common/st2common/util/pack.py imports `.`, but Pants cannot safely infer a dependency because more than one target owns this module, so it is ambiguous which to use: ['contrib/core/__init__.py', 'contrib/examples/lib/__init__.py', 'tools/__init__.py'].
The file is different in each message, but the list of targets that "own" 
.
is the same. These files do not have any relative imports, so I don't know what it means by the file 
imports
.`` - Any suggestions on how to debug this?
Hmm. All of the files have one of these strings (with single or double quotes): 
.
..
I have string imports enabled, with min dots = 1. And I found my note about debugging that: 
pants python-dump-source-analysis --analysis-flavor=raw_dependency_inference <path(s) to file(s)> | jq '.[].resolved'
Odd. and there's one string that has 
# pants: no-infer-dep
, but the rust parser is still picking up the string.
I tried adding 
[python-infer].ignored_unowned_imports=[".", ".."]
but that is not used in the "ambiguous dependencies" code path.
b
Oh, sorry for the regressions. Thoughts @bitter-ability-32190 ? (Is there a test suite for the string import behaviour of the old parser that we could port across to maybe cover more of this?) And/or is it something you might be able to (partially) fix @proud-dentist-22844 ?
b
Every test we had for the old parser passes on the new parser, and then some. That was explicitly a goal in the porting.
FWIW all string imports should be "weak" and therefore shouldn't error if they aren't matched against anything.
p
A screen full of weak import warnings that cannot be ignored are effectively errors. 🤷
b
They shouldn't be warning either. 🤨
The whole point is that if they don't match anything, treat them like garbage
p
For some reason, pants thinks that 3 modules “own” the 
.
or 
..
modules, so it is saying the import is ambiguous.
b
Yeah I think thats the bug.
p
And then there is one other import where the ignore pragma wasn’t taking effect. My PR at least addresses that (I think).
b
If I had to guess, we might special case modules that look like relative paths?
p
special case where? Ignore string imports for them (the approach I took)? Or somewhere else?
b
So either there's a bug in the vanilla weak import shouldn't warn logic, or were special casing certain strings and they special casing isn't respecting weakness
So like, if you add a random string to that module "flodbrujr.jfnrieo". Does that also warn?
p
Hmm. I don’t think so. checking …
b
That'd be a huge clue
p
57 files have the string 
.
4 files have the string 
..
My 
asdf.foobar
string did not cause a warning.
Maybe we should add 
.
and 
..
to the list of unownable modules?
DEFAULT_UNOWNED_DEPENDENCIES
is not consulted until after the ambiguous dependency warning is raised. Besides, adding 
.
or 
..
to it would not help because it is used like this:
Copy code
elif import_name.split(".")[0] in DEFAULT_UNOWNED_DEPENDENCIES:
I tried adding it to 
[python-infer].ignored_unowned_imports
, but that also did not help, since that doesn’t get checked until a couple rules later in the rule graph (also after the ambiguous dependency warnings). So, where would such special casing go?
b
Sorry, I meant we might already be special casing, not that we should special case
So why wasn't asdf warned about, but "." Was? That's what I'm missing
p
Because nothing seems like it should own 
asdf.foobar
. But pants thinks three modules own 
.
Here is one of the warnings:
Copy code
19:38:50.41 [WARN] The target st2client/st2client/commands/action.py imports `.`, but Pants cannot safely infer a dependency because more than one target owns this module, so it is ambiguous which to use: ['contrib/core/__init__.py', 'contrib/examples/lib/__init__.py', 'tools/__init__.py'].

Please explicitly include the dependency you want in the `dependencies` field of st2client/st2client/commands/action.py, or ignore the ones you do not want by prefixing with `!` or `!!` so that one or no targets are left.

Alternatively, you can remove the ambiguity by deleting/changing some of the targets so that only 1 target owns this module. Refer to <https://www.pantsbuild.org/v2.18/docs/troubleshooting#import-errors-and-missing-dependencies>.
b
And why is that?
p
I haven't figured that out yet. How can any module "own" 
.
? Esp in a project that doesn't use relative imports.
b
Yup, you now have my mental model as well 😁
p
Each of the modules that “own” 
.
are roots, but that’s only 3 of the 36 roots in the repo, so I’m not sure that tells me anything.
The same 3 files/roots “own” both 
.
and 
..
. There is an 
__init__.py
file in only 5 of the roots (all empty files). Of the 2 roots that do not “own” 
.
, 1 does not have any other python files, and the other 1 is in a different resolve. So, source roots that include an 
__init__.py
file and at least one other python file all own 
.
.
@bitter-ability-32190 What would you think of adding a check after this line: https://github.com/pantsbuild/pants/blob/694ea039126ec551601207d89458f0156a839002/[…]thon/pants/backend/python/dependency_inference/module_mapper.py
Copy code
module = module_from_stripped_path(stripped_f)
        if set(module) == {"."}:
            # Even if a source root has an __init__.py file,
            # it should not own "." or ".." or similar.
            continue
Any relative paths in dependencies or other fields are normalized before they get to the module owner stuff. So, the only way we can go looking for the 
.
module is via the string imports. I'm confident that we should exclude strings like 
.
and 
..
, but I'm not sure if there's ever a reason to look up roots that have python files. So, I still like protecting the string imports as I'm not sure the implications of preventing anything from owning the 
.
module 😕
b
🤔
p
I documented what I found in my PR.
34 Views
Open in Slack
PreviousNext
Powered by