Add --unowned-dependency-behavior option for Python #13491

thejcannon · 2021-11-04T21:42:29Z

As discussed in Slack, adding a way to flag deps that are silently unowned.

thejcannon · 2021-11-04T21:42:45Z

I'll try and add some unit tests soon. Local testing shows it works! 🎉

thejcannon · 2021-11-04T21:43:07Z

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

+# Copyright 2021 Pants project contributors (see CONTRIBUTORS.md).
+# Licensed under the Apache License, Version 2.0 (see LICENSE).
+
+_STDLIB_MODULES = [


Are there any modules that were removed from previous versions and might not appear? 🤔

@Eric-Arellano will likely have thoughts on this. We actually... removed per-interpreter-version lists a while back in #12818. But that was because we were using it to filter things, rather than to backstop a validation like this one.

Yeah I think this is an ever-growing list unless there's a good reason why it shouldn't be.

My question here is basically I just dumped the std library list form 3.10 because it was easy, so is there anything that needs to be added because 3.10 doesn't have them (anymore).

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

stuhood

Looks great! Thanks a lot.

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

stuhood · 2021-11-05T03:17:54Z

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

+        register(
+            "--unowned-dependency-behavior",
+            type=UnownedDependencyUsage,
+            default=UnownedDependencyUsage.DoNothing,


This is a good+safe initial default, but I'm very interested in how many false positives you get with this... if we're able to make it warn by default in a future release, that would be great.

Agreed! Will report back later on our monorepo

So far it's just found a bunch of questionable imports, things that belong in module_mapping, and imports I hadn't realized we had that'll need some finagling.

An example of the last one was a dependency on an in-house compiled C extension module. Or funny import tricks like:

try: import foo except ImportError: import bar

Just finished with the monorepo. Nothing too surprising. The only one that got me was an import like foo.bar.baz being unowned. I added a BUILD file to ./foo/bar (no BUILD files any higher) and declared a python_requirement with module "baz" (just to silence the warning, wouldn't pass the test) and still saw the error.

Instead I had to declare the module as "foo.bar.baz". Not sure if that change makes sense, or works around Pants' shortcomings in some way 🤔

I added a BUILD file to ./foo/bar (no BUILD files any higher) and declared a python_requirement with module "baz" (just to silence the warning, wouldn't pass the test) and still saw the error.

I think that this makes sense... python_requirement targets do not have source roots applied, so foo.bar would not have been prefixed onto baz in this case.

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

stuhood

Thanks, looks good!

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

Unfortunately, it looks like `--no-print-stacktrace` broke in a critical location a while back, resulting in unnecessary noise. See for example: #13491 (comment) Fix, and add an integration test. Also fixes the location of the `exceptions.log`, and removes `ExceptionSink` `Ctrl+C` tests, since `Ctrl+C` is now directly handled by the engine (fixes #12108). [ci skip-rust] [ci skip-build-wheels]

Unfortunately, it looks like `--no-print-stacktrace` broke in a critical location a while back, resulting in unnecessary noise. See for example: pantsbuild#13491 (comment) Fix, and add an integration test. Also fixes the location of the `exceptions.log`, and removes `ExceptionSink` `Ctrl+C` tests, since `Ctrl+C` is now directly handled by the engine (fixes pantsbuild#12108). [ci skip-rust] [ci skip-build-wheels]

Unfortunately, it looks like `--no-print-stacktrace` broke in a critical location a while back, resulting in unnecessary noise. See for example: #13491 (comment) Fix, and add an integration test. Also fixes the location of the `exceptions.log`, and removes `ExceptionSink` `Ctrl+C` tests, since `Ctrl+C` is now directly handled by the engine (fixes #12108). [ci skip-rust] [ci skip-build-wheels]

stuhood · 2021-11-09T18:13:53Z

We can probably land this whenever it's green in CI. If you need help with that, let me know!

thejcannon · 2021-11-09T18:20:13Z

Oh I wouldn't want that until I've added tests 😁 💪

Also I think we still want the whitelist support.

thejcannon · 2021-11-09T22:28:04Z

Dropped the "Draft" status after adding tests. 🚀

thejcannon · 2021-11-10T01:24:32Z

Actually, I want to add a few more tests, for some good coverage 👍

thejcannon · 2021-11-10T16:19:57Z

Officially good to go!

stuhood

Thanks!

CONTRIBUTORS.md

stuhood · 2021-11-10T17:37:59Z

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

+        register(
+            "--unowned-dependency-behavior",
+            type=UnownedDependencyUsage,
+            default=UnownedDependencyUsage.DoNothing,


I added a BUILD file to ./foo/bar (no BUILD files any higher) and declared a python_requirement with module "baz" (just to silence the warning, wouldn't pass the test) and still saw the error.

I think that this makes sense... python_requirement targets do not have source roots applied, so foo.bar would not have been prefixed onto baz in this case.

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

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Co-authored-by: Stu Hood <stuhood@gmail.com> # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Co-authored-by: Stu Hood <stuhood@gmail.com> # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

[ci skip-rust] [ci skip-build-wheels]

thejcannon commented Nov 4, 2021

View reviewed changes

stuhood reviewed Nov 5, 2021

View reviewed changes

stuhood mentioned this pull request Nov 8, 2021

Fix --no-print-stacktrace. #13539

Merged

stuhood approved these changes Nov 8, 2021

View reviewed changes

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

stuhood mentioned this pull request Nov 9, 2021

Fix --no-print-stacktrace. (cherrypick of #13539) #13540

Merged

stuhood mentioned this pull request Nov 9, 2021

Fix --no-print-stacktrace. (cherrypick of #13539) #13541

Merged

thejcannon marked this pull request as ready for review November 9, 2021 22:27

stuhood changed the title ~~WIP: Add --unowned-dependency-behavior option~~ Add --unowned-dependency-behavior option Nov 10, 2021

stuhood approved these changes Nov 10, 2021

View reviewed changes

stuhood changed the title ~~Add --unowned-dependency-behavior option~~ Add --unowned-dependency-behavior option for Python Nov 10, 2021

stuhood enabled auto-merge (squash) November 10, 2021 22:34

thejcannon added 8 commits November 10, 2021 16:20

Add --unowned-dependency-behavior option

d099ebb

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

fmt and lint

6058fbd

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

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

ef86bae

Co-authored-by: Stu Hood <stuhood@gmail.com> # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

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

aa8228d

Co-authored-by: Stu Hood <stuhood@gmail.com> # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

better output

7317de8

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Add address

a2b5e35

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

warning

1283f2b

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

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

8b83862

Co-authored-by: Stu Hood <stuhood@gmail.com> # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

thejcannon and others added 9 commits November 10, 2021 16:20

Tests!

79d7486

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

I'm a contributor!

bdd0313

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

fmt

25e3f95

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

mypy

63a3fdc

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

More tests

f4d29e8

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

use non-deprecated arg

107ff01

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Latest changes

6dc4f6f

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

quotes

c94e025

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Add fstring markers, and fix imports.

8aeb6f1

[ci skip-rust] [ci skip-build-wheels]

stuhood force-pushed the strict_constraints branch from 2753635 to 8aeb6f1 Compare November 11, 2021 00:28

stuhood merged commit 7f1dd56 into pantsbuild:main Nov 11, 2021

thejcannon deleted the strict_constraints branch November 11, 2021 14:14

jsirois mentioned this pull request Jan 22, 2023

Pants reports tomllib as a unowned dependency for a Python 3.11.* IC. #18055

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --unowned-dependency-behavior option for Python #13491

Add --unowned-dependency-behavior option for Python #13491

thejcannon commented Nov 4, 2021

thejcannon commented Nov 4, 2021

thejcannon Nov 4, 2021

stuhood Nov 5, 2021

thejcannon Nov 5, 2021

stuhood left a comment •

edited

Loading

stuhood Nov 5, 2021

thejcannon Nov 5, 2021

thejcannon Nov 9, 2021

thejcannon Nov 9, 2021

stuhood Nov 10, 2021

stuhood left a comment

stuhood commented Nov 9, 2021

thejcannon commented Nov 9, 2021

thejcannon commented Nov 9, 2021

thejcannon commented Nov 10, 2021

thejcannon commented Nov 10, 2021

stuhood left a comment

stuhood Nov 10, 2021

Add --unowned-dependency-behavior option for Python #13491

Add --unowned-dependency-behavior option for Python #13491

Conversation

thejcannon commented Nov 4, 2021

thejcannon commented Nov 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood left a comment • edited Loading

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

stuhood left a comment

Choose a reason for hiding this comment

stuhood commented Nov 9, 2021

thejcannon commented Nov 9, 2021

thejcannon commented Nov 9, 2021

thejcannon commented Nov 10, 2021

thejcannon commented Nov 10, 2021

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood left a comment •

edited

Loading