Dependees (dependents) should return dependent files for removed modules

[njgrisafi]

Is your feature request related to a problem? Please describe.
Related to this bug report I opened but changed it into a feature request.

When removing a whole Python module dependees (now dependents) does not return dependent files. On the latest version 2.14 and it seems to be a warning:

10:28:00.15 [WARN] Pants cannot infer owners for the following imports in the target app/module_1/example.py:

  * app.module_2.example.test (line: 1)

On earlier versions it returns no output.

Describe the solution you'd like
When removing modules and then running ./pants dependees --changed-since (or dependents), dependent targets of the removed modules will be returned in the output.

For example, given this module:

app/
  __init__.py
  module_1/
      __init__.py
      example.py (imports app.module_2.example)
      BUILD
  module_2/
      __init__.py
      example.py
      BUILD

When removing app/module_2/, add changes in git and then running ./pants dependees --changed-since=HEAD it should return app/module_1/example.py in the results.

Describe alternatives you've considered
An additional layer of tooling over pants to detect these issues. Custom logic to account for this. Open to more ideas.

Additional info
Please see example repo with predefined version branches:

• https://github.com/njgrisafi/pants-example

Checkout one of the follow version branches above and perform the following test:

1. rm -rf app/module_2/
2. git add --all
3. ./pants dependees --changed-since=HEAD

[benjyw]

To be sure I understand this example:

When you removed app/module_2/ then presumably you had to edit app/module_1/example.py to remove its import from app/module_2, so wouldn't it then show up in --changed-since for that reason?

[njgrisafi]

To be sure I understand this example:

When you removed app/module_2/ then presumably you had to edit app/module_1/example.py to remove its import from app/module_2, so wouldn't it then show up in --changed-since for that reason?

This is just removing the app/module_2/ directory without removing the import.

Take a CI run for example. A PR pushes code which removes app/module_2, we gather which tests to run and CI passes (though we did not run the tests in app/module_1). The PR is merged and breaks app/module_1/ tests on master.

An option I can think of is having some process outside of Pants (lint tool / typechecking) that can protect against these types of changes.

[benjyw]

Ah, I see, so this would be for, e.g., detecting that bad import in the first place.

[benjyw]

So one immediate option, for now, is to run dep inference on the entire repo (which should be cheap with caching) and look for WARNings about imports that couldn't be resolved.

[benjyw]

But also, this could be made to work as you expect by adding module mappings for deleted files. Might require synthesizing targets for them (so @kaos's new synthetic targets feature might come in handy!)

[stuhood]

Very sorry for the trouble. This is a dupe of #14975.

Our conclusion on that issue was that https://www.pantsbuild.org/v2.14/docs/reference-python-infer#section-unowned-dependency-behavior is critical, which is why it became a warning in the 2.14 release.

While we probably can't set the default to error (because that would make onboarding for new users more challenging), we would definitely recommend it for established repositories. Who knows though... maybe it's worth essentially calling that "lenient dependency mode" and warning folks that they are in lenient dependency mode until they set it to error.

[njgrisafi]

@stuhood thank you for the response, definitely is related! I changed [python-infer].unowned_dependency_behavior = "error" and it outputs the error as expected. However, it may be too invasive to flip this setting for a large project.

The error is better than potentially missing a test in CI but I'm curious if there's a seamless way to have Pants handle these cases out of the box.

[benjyw]

@njgrisafi do you have such warnings in your codebase that you're currently suppressing?

[benjyw]

I'd like to tease out the practical distinction, from a user perspective, of erroring during dep resolution vs during a test run, since in both cases the failure is due to the same cause - an import that has no matching provider.