https://pantsbuild.org/ logo
#general
Title
# general
a

ambitious-student-81104

09/17/2021, 5:58 PM
Hi Pants! I was wondering if you can add a feature to error out specifically for circular dependencies. This is something we typically run into, especially for people new to build systems; it's ambiguous when we're using automatic dependencies and on the error line it just says
Copy code
E   ImportError: cannot import name 'xxx' from 'xxxmodule' (path/to/xxx.py)
It will be much more helpful if the error actually says "circular dependency detected: file name 1, file name 2"
h

hundreds-father-404

09/17/2021, 6:03 PM
That's interesting, relates to https://github.com/pantsbuild/pants/issues/12871 We actually put a lot of work into tolerating cycles, which Pants used to choke on. Why? It can be legal in Python, like this:
Copy code
if TYPE_CHECKING:
     import my_circular_dependency
(Which is how you handle circular deps with MyPy type hints, that the cycle only happens when type checking which MyPy can handle, but not prod.) -- Would you want to always ban cycles no matter what? Or be able to control it more fine-grained, like some files can opt-into cycle tolerance?
w

witty-crayon-22786

09/17/2021, 6:36 PM
@ambitious-student-81104: to be clear: that error comes from Python itself, and as Eric mentioned, it’s possible to resolve it via conditional imports… but Pants could probably parse Python’s output to enrich it a bit?
(…only in some situations though, like in tests)
a

ambitious-student-81104

09/17/2021, 6:37 PM
Oh, to be clear, I'm asking if Pants can enrich the error message, NOT that it processes the error somehow (actually i would not like that... circular dependencies SHOULD result in error, and code authors should write clean code that doesn't have circular dependencies. Personally I do not like hidden imports either. it's one of the worse ways to address circular dependency issue.)
Reason is our repo collaborators aren't all super savvy about what to do with this kind of stuff (i wrote a readme FAQ telling ppl how to find out if it's due to circular dependency and how to address it, but it'd be better if the error message could just include
looks like you have a circular dependency: A-> B -> A
👍 1
w

witty-crayon-22786

09/17/2021, 6:48 PM
this is an interesting situation for sure!… Pants actually explicitly allows cycles between python code currently, because as mentioned above, it’s legal in cases where you’ve used conditional imports (likewise, other languages like Java allow import cycles)
so that means that rather than erroring while building your dependencies, we compute them successfully, and let the Python interpreter render the error.
but i agree: the error that Python renders isn’t great
1
but to enrich the error from Python we’d have to parse it
a

ambitious-student-81104

09/17/2021, 6:50 PM
legal in cases where you’ve used conditional imports (likewise, other languages like Java allow import cycles)
we typically encourage people to write imports only at file top level. This is much more readable. Most conditional imports can be resolved by re-structuring the code, which ends up in more readable, top-level import only code
w

witty-crayon-22786

09/17/2021, 6:50 PM
it’s possible that the answer is making it optional to ban import cycles in dependencies: that would allow for an error earlier.
yep: i agree!
h

hundreds-father-404

09/17/2021, 6:51 PM
Stu, or let users control the behavior: they can disable pants cycle tolerance. My question above is how that should be modeled, if a global toggle is enough vs per language settings vs per target settings
And that relates to the ticket I linked to in that we already will need to redesign cycle tolerance code to allow languages to express their semantics, like go banning it but python allowing. So this feature request could fit in naturally
w

witty-crayon-22786

09/17/2021, 6:52 PM
@ambitious-student-81104: would you mind opening a ticket about this? there are other changes we’re planning to make (https://github.com/pantsbuild/pants/issues/12871), and we should be able to either add an option, detect the case and enrich the error, or figure out how not to have false positives earlier in construction
@hundreds-father-404: yep, unclear.
@ambitious-student-81104: does your repo declare many explicit
dependencies
in
BUILD
files, or are you mostly using dependency inference?
a

ambitious-student-81104

09/17/2021, 6:54 PM
Sounds good. Thanks for the context! Do you want me to just open a new issue?
w

witty-crayon-22786

09/17/2021, 6:54 PM
yea, that would be great: thank you!
a

ambitious-student-81104

09/17/2021, 6:54 PM
Mostly using dependency inference, which is why it's extra-ambiguous to users not BUILD-savvy. They only see the ImportError, they might not even think that it might be a circular dependency problem, because there's no
dependencies=[...]
w

witty-crayon-22786

09/17/2021, 6:55 PM
the import statements act as the dependencies list in this case, but yea: got it.
a

ambitious-student-81104

09/17/2021, 7:43 PM
h

happy-kitchen-89482

09/17/2021, 11:10 PM
Yeah, I agree that instead of enhancing the error message, we should allow you to opt out of cycle tolerance, and error early
1
h

hundreds-carpet-28072

02/27/2024, 5:01 PM
I’m interested in the option of opt-out from Pants’ cycle tolerance. Has there been any more movement/discussion towards this more recently?
h

happy-kitchen-89482

02/27/2024, 9:30 PM
There has not, but that is good info to have.
You want pants to enforce a lack of cycles? There is a different feature that may be useful for this: https://www.pantsbuild.org/2.19/docs/using-pants/validating-dependencies
h

hundreds-carpet-28072

02/28/2024, 10:47 AM
I was aware of the visibility rules but need to take a look into them, that may help, thanks! An additional nice feature would be to be able to detect cycles based on target type also. For instance for
python_distribution
targets it would be useful to identify cycles between distributed packages, not necessarily their source files, as this could inform defining distributions and prevent pip conflicts. If that makes sense?
4 Views