Add dependencies on conftest and init files via inference #10441

stuhood · 2020-07-24T00:21:02Z

Problem

When attempting to release dev5 internally, many conftest.py files were being included without their dependencies by the pytest_runner conftest.py injection that was added in #10378.

Because that injection was not actually declaring dependencies on those files, it 1) wasn't locating owning targets, or 2) triggering dep inference on those files. This meant that even though we could successfully infer the deps of the conftest.py files, we weren't... just including them verbatim.

Solution

Move to inferring dependencies on conftest.py and __init__.py files, which will cause the dependencies of those files to be included, and cause them to be reported in our graph introspection goals.

Each thing that we can infer is provided by a separate inference @rule in @union InferDependenciesRequest, so this change also enables multiple inference rules per type of Sources.

Result

conftest.py files are included along with their dependencies, fixing all known test failures in dev5.

stuhood · 2020-07-24T00:25:27Z

The commits are useful to review independently, but this is currently a draft for two reasons:

The third commit adds owning targets for __init__.py files that weren't owned (which would otherwise fail due to being partially included in mypy).
- We should think about whether we want to just delete all of our __init__.py files instead, since they are all empty.
- Also notable: internally, this change only required 3 BUILD file edits... conftest.py and __init__.py are overwhelmingly already owned by targets.
We probably want options to toggle each inference implementation independently: it's possible that we want __init__.py and conftest.py inference each on by default, but not "python inference" in general.

benjyw · 2020-07-24T01:01:02Z

I tend to agree that we should nuke our own __init__.py files. But this exposes an issue: in practice existing python_library() targets will glob over __init__.py. If people aren't using dep inference they must now manually add a dep to that target, and they may end up dragging in a TON of stuff they don't need, and possibly cause cycles, even if the __init__.py is empty. With dep inference and file-level deps on I guess this isn't a problem. This seems like an unavoidable consequence of the fact that __init__.py has two roles : "Just a regular Python source file" and "thing that indicates that this dir represents a package, and is executed when you import that package". So really the healthy way to model code in a dir is to always have a separate target for __init__.py.

benjyw · 2020-07-24T01:01:43Z

Re separate toggles - possibly, yes, otherwise we're requiring people to write manual deps on parent __init__.py, which is unintuitive.

stuhood · 2020-07-24T01:08:16Z

If people aren't using dep inference they must now manually add a dep to that target, and they may end up dragging in a TON of stuff they don't need, and possibly cause cycles, even if the init.py is empty.

Well, see my second point above: I think that this inference rule should be on by default (since we effectively already turned it on by default by landing the "inject" stuff). But also, because this a file-level dependency, 1) it won't drag in any more files than you need, 2) it won't cause cycles because of #10409.

This seems like an unavoidable consequence of the fact that init.py has two roles : "Just a regular Python source file" and "thing that indicates that this dir represents a package, and is executed when you import that package".

Or not have them at all. In our case, there is zero need for them post PEP 420 (afaict: will confirm before landing). If that's true, we can delete all of them in a followup.

EDIT: It looks like __init__.py files are still somewhat necessary for mypy, even with namespace_packages turned on? Will want to revisit whether they can be removed in the future, but doesn't feel like a blocker for now.

Eric-Arellano

Thanks.

How would you feel about keeping the prior behavior for __init__.py and pytest_runner.py in addition to your change? The way AncestorFiles is implemented, if you have dep inference, we won't end up trying to re-find those missing files because they are already in prepared_sources.snapshot. So, dep inference will behave the same as you're intending.

For people not using dep inference, they'll get the injection. This avoids the problems Benjy is concerned about, like having to create lots of new targets and having new cycles.

src/python/pants/backend/python/dependency_inference/rules.py

src/python/pants/engine/internals/graph.py

stuhood · 2020-07-24T02:50:30Z

How would you feel about keeping the prior behavior for init.py and pytest_runner.py in addition to your change? The way AncestorFiles is implemented, if you have dep inference, we won't end up trying to re-find those missing files because they are already in prepared_sources.snapshot. So, dep inference will behave the same as you're intending.

That wouldn't fix the bug I don't think: see the Problem description. Basically, injecting the conftest.py files where we were was too late to have their dependencies inferred.

I think that __init__.py files that had imports would be fine for the reason you mention (they would be injected before inference), but it doesn't feel worthwhile to have file injection as a special case rather than using the framework that we already have for adding dependencies. And under the hood it means that we have a bunch of secretly multiple-owned files, which is confusing/magical.

The thing that would feel cleanest to me would be to have inference of both conftest.py and __init__.py on by default, but to continue to have the existing python inference off by default. See item 2 from #10441 (comment) ... if that seems reasonable to everyone.

benjyw · 2020-07-24T03:31:58Z

How would you feel about keeping the prior behavior for __init__.py and pytest_runner.py in addition to your change? The way AncestorFiles is implemented, if you have dep inference, we won't end up trying to re-find those missing files because they are already in prepared_sources.snapshot. So, dep inference will behave the same as you're intending.

I don't think this works - if the __init__.py isn't empty and does imports, we have to follow its imports, so the prior behavior was never correct.

benjyw · 2020-07-24T03:32:36Z

The thing that would feel cleanest to me would be to have inference of both conftest.py and __init__.py on by default, but to continue to have the existing python inference off by default. See item 2 from #10441 (comment) ... if that seems reasonable to everyone.

This seems reasonable to me.

benjyw · 2020-07-24T03:34:09Z

Not so sure about there being zero need for __init__.py in pep420. An empty __init__.py is still required in order to specify that a directory is a regular package. A missing __init__.py says that a dir is a namespace package, which may not be what you want.

For example, various parts of Django choke on namespace packages in your code.

stuhood · 2020-07-24T07:08:15Z

This is reviewable. The commits are still useful to look at independently.

Eric-Arellano

Thanks. Good idea to have 3 new options.

Eric-Arellano · 2020-07-24T16:40:43Z

src/python/pants/backend/python/dependency_inference/rules.py

+class PythonInference(Subsystem):
+    """Options controlling which dependencies will be inferred for Python targets."""
+
+    options_scope = "python-infer"


I think generally we have subsystems be nouns. I'd recommend python-inference.

Good idea to have a dedicated subsystem, btw.

I started with that, but the option names were so much nicer with the verb... --python-infer-imports, --no-python-infer-imports, etc. Should I change the option names as well?

True, but this gets to Benjy's point that 90% of options are only ever set in pants.toml. I strongly suspect this will only be set in pants.toml, outside of a codebase admin experimenting with things.

So, it would read:

[python-inference] imports = true inits = true conftests = true

I kinda like [python-infer] even in config TBH

Leaning toward leaving it then.

src/python/pants/backend/python/dependency_inference/module_mapper.py

src/python/pants/backend/python/typecheck/mypy/rules_integration_test.py

src/python/pants/engine/internals/graph.py

src/python/pants/engine/internals/graph_test.py

tests/python/pants_test/releases/reversion_integration_test.py

Eric-Arellano

Thank you. I have no idea what happened to isort for you?

src/python/pants/backend/python/dependency_inference/rules_test.py

src/python/pants/backend/python/rules/pex.py

src/python/pants/backend/python/dependency_inference/module_mapper_test.py

src/python/pants/backend/python/rules/pex_from_targets.py

src/python/pants/backend/python/rules/pex_test.py

src/python/pants/backend/python/rules/util.py

src/python/pants/backend/python/rules/run_setup_py_test.py

src/python/pants/backend/python/rules/util_test.py

src/python/pants/engine/internals/graph_test.py

Eric-Arellano

Great work!

Eric-Arellano · 2020-07-24T20:00:38Z

src/python/pants/engine/internals/graph.py

@@ -740,8 +763,8 @@ async def resolve_dependencies(
    )

    inference_request_types = union_membership.get(InferDependenciesRequest)
-    inferred = InferredDependencies()
-    if global_options.options.dependency_inference and inference_request_types:
+    inferred: Tuple[InferredDependencies, ...] = (InferredDependencies(),)


Nit. It's fine to fix this in a followup, I know we're trying to land this for the release.

Suggested change

inferred: Tuple[InferredDependencies, ...] = (InferredDependencies(),)

inferred = ()

Eric-Arellano · 2020-07-24T20:03:53Z

testprojects/src/python/unicode/compilation_failure/BUILD

+python_library(
+  name='lib',
+  sources = ['__init__.py'],
+  tags = {"nolint"},
 )


Ohhh I see the bug now! So, the __init__ inference rule found that there is indeed an owner of this file, but that owner isn't an actual Python target. It would be more robust if we instead had the inference rule filter out all the resulting owners to check that .has_field(PythonSources) is true.

Without this target definition added, we would have wanted the __init__.py inference rule to error that there is no Python owner of this file.

--

It's fine to fix this in a followup, imo, so long as we do fix it.

Eric-Arellano · 2020-07-24T20:07:03Z

Hm, do note that this may result in far more dependencies being pulled in than previously if you are not using --python-infer-imports. Even though this will create a file level dep on the __init__.py file, that generated subtarget will copy over its parent's entire dependencies field. While we will tolerate any possible cycles, our invalidation will be much worse.

I'm not really sure what the remedy would be here. I'm glad that there is an escape hatch with turning off this feature.

coveralls · 2020-07-24T20:50:56Z

Coverage remained the same at 0.0% when pulling 2a409be on stuhood:stuhood/conftest-and-init-via-inference into 864e77c on pantsbuild:master.

benjyw · 2020-07-24T21:44:03Z

Hm, do note that this may result in far more dependencies being pulled in than previously if you are not using --python-infer-imports. Even though this will create a file level dep on the __init__.py file, that generated subtarget will copy over its parent's entire dependencies field. While we will tolerate any possible cycles, our invalidation will be much worse.

I'm not really sure what the remedy would be here. I'm glad that there is an escape hatch with turning off this feature.

The only solution that is robust in this case is to have a separate target for __init__.py...

stuhood · 2020-07-27T04:35:38Z

Hm, do note that this may result in far more dependencies being pulled in than previously if you are not using --python-infer-imports. Even though this will create a file level dep on the __init__.py file, that generated subtarget will copy over its parent's entire dependencies field. While we will tolerate any possible cycles, our invalidation will be much worse.
I'm not really sure what the remedy would be here. I'm glad that there is an escape hatch with turning off this feature.

The only solution that is robust in this case is to have a separate target for __init__.py...

Keep in mind too that under inference, the dep lists of most targets in BUILD files should be nearly empty: in that case, the effect of this change would be getting only the __init__.py file, and any inferred imports it contained.

Eric-Arellano · 2020-07-27T04:39:23Z

Yes, agreed. I am describing the case where you are not using import inference. Mostly V1 users upgrading to V2.

[ci skip-rust-tests]

…n-infer subsystem. [ci skip-rust-tests]

[ci skip-rust-tests]

# Rust tests will be skipped. Delete if not intended. [ci skip-rust-tests]

Eric-Arellano · 2020-07-27T16:39:07Z

src/python/pants/backend/python/rules/python_sources.py

@@ -4,9 +4,6 @@
 from dataclasses import dataclass


The docstring is out of date in this file. It should remove all mention of __init__.py.

) ### Problem #10441 caused a performance regression from about 10s to run `./pants dependencies --transitive ::` with inference enabled, to about 22s. ### Solution Make independent owners requests per file we'd like to find owners, which allows the lookups for each file to be memoized independently. ### Result `./pants dependencies --transitive ::` takes 15s. Although we are doing more work than before (before #10441, conftest discovery would only happen at `test` time), this is not ideal, and we should do further optimization before launch. But there are a few variables that will impact this soon that make it not the best time to optimize: 1) an intrinisic `PathGlobs->Paths` operation, 2) possibly enabling inference by default, allowing all of the strategies to use the module_mapper. [ci skip-rust-tests]

stuhood requested review from benjyw and Eric-Arellano July 24, 2020 00:21

Eric-Arellano reviewed Jul 24, 2020

View reviewed changes

src/python/pants/backend/python/dependency_inference/rules.py Show resolved Hide resolved

src/python/pants/engine/internals/graph.py Show resolved Hide resolved

stuhood marked this pull request as ready for review July 24, 2020 07:08

Eric-Arellano reviewed Jul 24, 2020

View reviewed changes

stuhood force-pushed the stuhood/conftest-and-init-via-inference branch from 0e4b382 to 55b9a54 Compare July 24, 2020 19:15

Eric-Arellano reviewed Jul 24, 2020

View reviewed changes

Eric-Arellano approved these changes Jul 24, 2020

View reviewed changes

stuhood force-pushed the stuhood/conftest-and-init-via-inference branch from 7437cce to d12ffd7 Compare July 27, 2020 04:28

stuhood force-pushed the stuhood/conftest-and-init-via-inference branch from d12ffd7 to fccb6b3 Compare July 27, 2020 15:11

stuhood added 6 commits July 27, 2020 09:17

Move init and conftest injection to dependency inference rules.

ce143d8

[ci skip-rust-tests]

Allow OwnersRequest to directly fail for unowned files.

4c829b9

[ci skip-rust-tests]

Add targets to cover our existing __init__.py files.

ab32daa

[ci skip-rust-tests]

Deprecate the global --dependency-inference option, and add the pytho…

335c762

…n-infer subsystem. [ci skip-rust-tests]

Test fixes.

175a62a

[ci skip-rust-tests]

Review feedback.

a897492

# Rust tests will be skipped. Delete if not intended. [ci skip-rust-tests]

stuhood added 2 commits July 27, 2020 09:17

Fix isort

2b6ad5d

# Rust tests will be skipped. Delete if not intended. [ci skip-rust-tests]

Review feedback and a test fix.

2a409be

# Rust tests will be skipped. Delete if not intended. [ci skip-rust-tests]

stuhood force-pushed the stuhood/conftest-and-init-via-inference branch from fccb6b3 to 2a409be Compare July 27, 2020 16:18

Eric-Arellano reviewed Jul 27, 2020

View reviewed changes

stuhood mentioned this pull request Jul 27, 2020

__init__ and conftest inference will pull in non-Python owners of those files #10467

Closed

stuhood merged commit 38bc143 into pantsbuild:master Jul 27, 2020

stuhood deleted the stuhood/conftest-and-init-via-inference branch July 27, 2020 17:18

This was referenced Jul 27, 2020

Cycles involving file addresses not always permitted #10471

Closed

Make independent owners requests per file to improve memoization #10491

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add dependencies on conftest and init files via inference #10441

Add dependencies on conftest and init files via inference #10441

stuhood commented Jul 24, 2020 •

edited

stuhood commented Jul 24, 2020 •

edited

benjyw commented Jul 24, 2020

benjyw commented Jul 24, 2020

stuhood commented Jul 24, 2020 •

edited

Eric-Arellano left a comment

stuhood commented Jul 24, 2020 •

edited

benjyw commented Jul 24, 2020

benjyw commented Jul 24, 2020

benjyw commented Jul 24, 2020

stuhood commented Jul 24, 2020 •

edited

Eric-Arellano left a comment

Eric-Arellano Jul 24, 2020

Eric-Arellano Jul 24, 2020

stuhood Jul 24, 2020

Eric-Arellano Jul 24, 2020

benjyw Jul 24, 2020

stuhood Jul 24, 2020

Eric-Arellano left a comment

Eric-Arellano left a comment

Eric-Arellano Jul 24, 2020

Eric-Arellano Jul 24, 2020

stuhood Jul 27, 2020

Eric-Arellano commented Jul 24, 2020

coveralls commented Jul 24, 2020 •

edited

benjyw commented Jul 24, 2020

stuhood commented Jul 27, 2020 •

edited

Eric-Arellano commented Jul 27, 2020

Eric-Arellano Jul 27, 2020

	inferred: Tuple[InferredDependencies, ...] = (InferredDependencies(),)
	inferred = ()

Add dependencies on conftest and init files via inference #10441

Add dependencies on conftest and init files via inference #10441

Conversation

stuhood commented Jul 24, 2020 • edited

Problem

Solution

Result

stuhood commented Jul 24, 2020 • edited

benjyw commented Jul 24, 2020

benjyw commented Jul 24, 2020

stuhood commented Jul 24, 2020 • edited

Eric-Arellano left a comment

Choose a reason for hiding this comment

stuhood commented Jul 24, 2020 • edited

benjyw commented Jul 24, 2020

benjyw commented Jul 24, 2020

benjyw commented Jul 24, 2020

stuhood commented Jul 24, 2020 • edited

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Arellano commented Jul 24, 2020

coveralls commented Jul 24, 2020 • edited

benjyw commented Jul 24, 2020

stuhood commented Jul 27, 2020 • edited

Eric-Arellano commented Jul 27, 2020

Choose a reason for hiding this comment

stuhood commented Jul 24, 2020 •

edited

stuhood commented Jul 24, 2020 •

edited

stuhood commented Jul 24, 2020 •

edited

stuhood commented Jul 24, 2020 •

edited

stuhood commented Jul 24, 2020 •

edited

coveralls commented Jul 24, 2020 •

edited

stuhood commented Jul 27, 2020 •

edited