https://pantsbuild.org/ logo
Join Slack
Powered by
I present the new error message if `[python-infer]...
# general
h
I present the new error message if 
[python-infer].unowned_dependency_behavior = "error"
, which errors when a dependency cannot be inferred. We're thinking of defaulting to warning or error because this error is hopefully much better than a cryptic runtime 
ModuleNotFoundErrror
Feedback welcomed!
✅ 1
UnownedDependencyError: Pants cannot infer owners for the following imports in the file src/python/pants/util/strutil_test.py (from the target src/python/pants/util/strutil_test.py:tests): • fake (line: 8) If you do not expect an import to be inferrable, add 
# pants: no-infer-dep
to the import line. Is the import from a third-party dependency (see https://www.pantsbuild.org/v2.12/docs/python-third-party-dependencies)? Some common issues: • Pants does not know about the requirement, i.e. there is not a 
python_requirement
target for it. You can run 
./pants filter --target-type=python_requirement ::
to see all 
python_requirement
targets. If you are using 
requirements.txt
, Poetry, or Pipenv, make sure you have 
python_requirements
, 
poetry_requirements
, and 
pipenv_requirements
target generators, respectively. Double check that the requirement is in the respective file like 
requirements.txt
. • The third-party requirement exposes modules different than Pants's default, e.g. 
ansicolors
exposing 
colors
. Set the 
modules
or 
module_mapping
fields. • Ambiguity. See below. Is the import from first-party code? Some common issues: • The file does not exist. • The file is missing an owning target like 
python_sources
. Run 
./pants list path/to/file.py
to see if there are any owners. If not, run 
./pants tailor
to generate targets. • The source root is not correctly set up, which is how Pants knows for example that the file 
src/py/my_project/app.py
exposes the module 
<http://my_project.app|my_project.app>
. See https://www.pantsbuild.org/v2.12/docs/source-roots. • Ambiguity. See below. Some common issues with both first and third-party imports: • Ambiguity. See above logs if there are warnings and instructions.
Thanks @bitter-ability-32190 for adding this super cool mechanism!
🙌 1
➕ 1
If we detect there are multiple resolves in the repo: Some common issues with both first and third-party imports: • Ambiguity. See above logs if there are warnings and instructions. • Multiple resolves. A target may exist for the import, but not share the same resolve of python-default from the target src/python/pants/util/strutil_test.py:tests. Pants only can infer dependencies on targets using the same resolve. You may need to set the 
resolve=
field for whichever target provides that import (and possibly use the 
parametrize
mechanism if that code should work with multiple resolves). See https://www.pantsbuild.org/v2.12/docs/python-third-party-dependencies#multiple-lockfiles.
I'm thinking of how we can improve the multiple resolves situation, like if we can dynamically detect if there is that import but inside another resolve
w
I’m thinking of how we can improve the multiple resolves situation, like if we can dynamically detect if there is that import but inside another resolve
we should i think… it’s really cheap to do.
h
yeah, just a UX question. Starting to try to draft the message
w
(if about to render the warning, check in other resolves, then include that in the message)
h
Would you agree that if the module is another resolve, that is realistically the only likely cause of the failure? We don't need to mention source roots, etc? I can't convince myself of that There is a lot of noise atm. I'm hoping we can detect this case eagerly and say "This is the issue!"
w
Would you agree that if the module is another resolve, that is realistically the only likely cause of the failure?
about a 95% chance, yea? there are overlap cases where it might be that you meant to use some sources instead of thirdparty, for example… but yea.
given that, is https://github.com/pantsbuild/pants/pull/15334 ready, or should we wait?
h
Hm maybe we still list the other reasons, but later on. "If that wasn't the issue, try these" John's wisdom about when suggestions aren't accurate seems prudent. That 5% case would be very confusing!
w
yep. that seems like a “if none of these are the module that you meant, then please see {doc_url(..)} for other possibilities” is sufficient in this case i think.
h
given that, is https://github.com/pantsbuild/pants/pull/15334 ready, or should we wait?
Totally depends how we want to approach the intersection of 
unowned_dependency_behavior
and multiple resolves: to get this helpful error message for multiple resolves, do you need to have on that option? Otherwise, we would need Yet Another warning/log and probably an option to toggle the behavior. Oof
The boat has sailed for deprecating default for 
unowned_dependency_behavior
in 2.11. But we can still make this improvement and then encourage users w/ multiple lockfiles to manually turn it on 2.12+ can deprecate the current default of ignore
w
where are we on enabling 
unowned_dependency_behavior
by default? i think that we should consider it.
h
so I am +1 to coupling 
unowned_dependency_behavior
w/ multiple resolves
where are we on enabling unowned_dependency_behavior by default? i think that we should consider it.
Extremely strong +1 from me. I think this is such a better experience than a ModuleError @bitter-ability-32190 over DM was +1, but not sure between error vs warn
w
i don’t know about coupling it per-se… but we should either 1) have it on by default, 2) strongly suggest it
IMO, until we have a way to render all of the errors, we shouldn’t make it an error by default. too much of a pain for new users.
h
i don’t know about coupling it per-se
The error message about multiple resolves has to come somewhere. What I'm suggested is it be included in the error message from 
unownded_dependency_behavior
. Do not invent yet another error message/log
w
agreed. i wasn’t thinking of that as “coupled”, but yea, got it.
👍 1
h
IMO, until we have a way to render all of the errors, we shouldn’t make it an error by default. too much of a pain for new users.
I wonder how we could pull that off. I think we'd need to make the dependency inference plugin hook have a fallible component where you can insert *lazy error messages?
w
I wonder how we could pull that off.
i’ve had some ideas recently (related to collecting atomic sideeffects via EngineAwareReturnType, and then committing them at the end of the run). but i don’t think that it needs to be tackled here.
👍 1
h
Either way, sg that I will improve this error message for multiple resolves now. Cherry-pick to 2.11. We can encourage users of multiple resolves to turn this on
👍 1
b
My 2c was that if it is annoying but disableable/helpful, it beats quiet-yet-might-hide-issues
👍 2
Agreed the repetitively iterative nature of the errors though is rough
h
Re new repositories and the default of this option: my thinking is that once you fix one issue, you are likely to fix many other. They are often structural issues: • bad source roots • missing targets • the idea of module_mapping
w
Agreed the repetitively iterative nature of the errors though is rough
yea: that’s why not errors until we solve that problem. mentioned above (and elaborated since) that i’ve had some thoughts on it
h
Agreed the repetitively iterative nature of the errors though is rough
On the other hand, what if you get 100 errors but fixing 1 would fix 90 of them. You give up because 100 is overwhelming?
We could say something like "Don't despair! Often fixing 1 fixes the others" 😅
b
In my experience each error was an individual issue, however there were duplicates. So solving 1 just solved one 🙈
❗ 1
w
the source root and thirdparty module ones that brand new users see will be super widespread and duplicated most likely.
➕ 2
i experienced it recently in a new repository with ambiguity warnings… on the JVM side, ambiguity is symbol level, so … even more redundant
i’ve had some ideas recently (related to collecting atomic sideeffects via EngineAwareReturnType, and then committing them at the end of the run).
elaborated here: https://github.com/pantsbuild/pants/issues/15350#issuecomment-1121536968
8 Views
Open in Slack
PreviousNext
Powered by