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

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"

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:

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?

@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?

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.)

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)

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

it’s possible that the answer is making it optional to ban import cycles in dependencies: that would allow for an error earlier.

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

@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

Sounds good. Thanks for the context! Do you want me to just open a new issue?

yea, that would be great: thank you!

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=[...]

the import statements act as the dependencies list in this case, but yea: got it.

Yeah, I agree that instead of enhancing the error message, we should allow you to opt out of cycle tolerance, and error early

I’m interested in the option of opt-out from Pants’ cycle tolerance. Has there been any more movement/discussion towards this more recently?

There has not, but that is good info to have.

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?