Add the ability to infer assets from strings for Python #14049

thejcannon · 2022-01-03T16:23:56Z

High level

Although less structured, it shouldn't be difficult to infer assets from strings and filter the noise to only what is valid.

The use-case should be evident, however here's a few intentional design decisions:

Ability to infer both resources (assumed to be relative to the module for now) and files (relative to the build root)
Scope the possible strings to a subset of what really is possible. On Unix, basically anything is a valid filename, but that could be needless explosive.
Use strings (as opposed to just parsing pkgutil.get_data) because some people use custom resource types. E.g. Resource("data/foo.txt"). We can add pkgutil.get_data support later.

Example:

Given:

<root>/
├─ configs/
│  ├─ prod.txt (a file)
├─ src/
│  ├─ app1/
│  │  ├─ data/
│  │  │  ├─ db.json (a resource)
│  │  ├─ app.py

(With all the right BUILD file plumbing)
Then src/app1/app.py containing code like open("configs/prod.txt") as envvars or Database("data/db.json") would have the relevant file/resource dependency inferred by Pants! 🎉

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

build-support/bin/generate_docs.py

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

thejcannon · 2022-01-05T14:23:12Z

Can someone re-kick the tests?

stuhood · 2022-01-05T17:25:08Z

Can someone re-kick the tests?

Will need to rebase I think: the fix for this landed on main as 5c10169.

thejcannon · 2022-01-05T17:57:59Z

I suppose rebasing is an easy re-kick. 🙌

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

thejcannon · 2022-01-07T02:10:08Z

src/python/pants/backend/python/dependency_inference/BUILD

-python_sources(
-    overrides={
-        "parse_python_imports.py": {
-            # This Python script is loaded as a resource, see parse_python_imports.py for more info.
-            "dependencies": ["./scripts:import_parser"]
-        }
-    }
-)


You might ask where this has gone. The answer: it's being inferred 🪄

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

kaos

Looking good. Haven't worked through it all in details, though.. :)

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

src/python/pants/backend/python/dependency_inference/scripts/dependency_parser.py

Eric-Arellano

Neat! Iirc, this would not handle this: importlib.resources.read_text("path.to.module", "foo.txt"), right? Even if not, this is still great.

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

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

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

src/python/pants/core/util_rules/resources.py

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

thejcannon · 2022-01-26T19:45:38Z

Neat! Iirc, this would not handle this: importlib.resources.read_text("path.to.module", "foo.txt"), right? Even if not, this is still great.

It does not. I didn't want to risk adding too many things to this PR, and import inference doesn't support importlib.import_module(). So I figured we can add support for both of these later.

Eric-Arellano · 2022-01-26T19:48:57Z

So I figured we can add support for both of these later.

Makes sense. Only thing we need to be careful about is how to handle backwards-compatibility. If we change the behavior of this new option to become more powerful / infer more than before, that is somewhat a breaking change. I think it's acceptable, but we should probably hedge in the help message by saying

Note that we hope to continue improving the inference of resources/files, such as eventually inferring importlib.resources statements. These improvements will happen without the normal deprecation policy.

thejcannon · 2022-01-26T20:12:02Z

So I figured we can add support for both of these later.

Makes sense. Only thing we need to be careful about is how to handle backwards-compatibility. If we change the behavior of this new option to become more powerful / infer more than before, that is somewhat a breaking change. I think it's acceptable, but we should probably hedge in the help message by saying

Note that we hope to continue improving the inference of resources/files, such as eventually inferring importlib.resources statements. These improvements will happen without the normal deprecation policy.

That's likely to be true of imports as well, right? With importlib.import_module?

Eric-Arellano · 2022-01-26T20:44:34Z

That's likely to be true of imports as well, right? With importlib.import_module?

Yes. I think we weren't considering how imports could be improved because we didn't consider importlib.import_module.

I'm pretty sure it doesn't break our deprecation policy, only a possible gotcha. Maybe the second sentence in my suggested language isn't necessary, but probably good to have the first one.

thejcannon · 2022-01-27T01:34:17Z

Actually this had made me realize, we might want a union rule for this, and split out the implementation into it's own rule. That also means maybe splitting the resource parsing out of the helper file into its own file.
FWIW this method is called infer_python_dependencies_via_imports, which is incorrect for this use case.

Yeah OK I think this is likely the route to take. I'll most likely post a new PR for that since the changes will be somewhat drastic. 🤞 hopefully there isn't too much duplication between this and import parsing.

Eric-Arellano · 2022-01-27T01:48:25Z

I don't think I agree with splitting out the parsing into two separate Processes — there is overhead to launching more Processes. Notably, they will both have the exact same input_digest, so there's no caching benefit to this.

Instead, it's important that the parser do the minimum work necessary. If you don't care about resources, then don't try to parse them. It looks like you already made that optimization.

we might want a union rule for this

That sounds like it might be more complex. What would be the benefit?

thejcannon · 2022-01-27T14:32:39Z

It's more about simplification and separation of concerns. The infer_python_dependencies_via_imports (which needs to be renamed, oops) already does a lot for python imports, and now we're adding more into it.

Perhaps splitting it into helpers (not necessarily needing #14238 but that'd be even nicer) would qualm some doubts.

For implementations sake, you might be right that bundling them together is still simpler.

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

thejcannon · 2022-03-03T16:57:01Z

OK I did another once-over on the PR. Should be good to re-review. Let me know if it needs to be split.

# 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]

Eric-Arellano

🥳

src/python/pants/backend/python/dependency_inference/scripts/dependency_parser.py

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

Eric-Arellano

Ability to infer both resources (assumed to be relative to the module, unless pkgutil.get_data is used) and files

Is the PR description still up-to-date? I'm not following the code for this part.

Generally, it seems that this only infers in the format path/to/f.txt, not path.to.my_resource, right?

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

module mapping as a rule

thejcannon · 2022-03-07T21:51:25Z

Ability to infer both resources (assumed to be relative to the module, unless pkgutil.get_data is used) and files

Is the PR description still up-to-date? I'm not following the code for this part.

Generally, it seems that this only infers in the format path/to/f.txt, not path.to.my_resource, right?

Fixed the wording

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

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

src/python/pants/backend/python/dependency_inference/scripts/dependency_parser.py

# 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]

Eric-Arellano · 2022-03-10T19:20:27Z

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

@@ -31,21 +32,37 @@ class ParsedPythonImports(FrozenDict[str, ParsedPythonImportInfo]):
    """All the discovered imports from a Python source file mapped to the relevant info."""


+class ParsedPythonAssets(DeduplicatedCollection[str]):


Maybe ParsedPythonAssetPaths? I continue to confuse myself with what this is, I think because of the old setup of it being a tuple.

Yeah. If/when we support pkgutil.get_data we'll be changing this name 😅 but for now that works!

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

Eric-Arellano · 2022-03-10T21:02:40Z

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

@@ -89,6 +103,24 @@ class PythonInferSubsystem(Subsystem):
            "treated as a potential dependency if this option is set to 2 but not if set to 3."
        ),
    )
+    assets = BoolOption(
+        "--assets",
+        default=False,


If you didn't have to consider our deprecation policy, should this default to True? We don't have string imports default to True because of false positives. Not sure what the right behavior is here.

Yeah. OK. I can get very behind making True the default.

Ah, but note that we do need to consider our deprecation policy, I think. Possibly. Unclear to me. I was only asking what the aspiration is.

In the interest of landing this for 2.10, how about using False for now, having a conversation in Slack or dedicated PR on Monday, and then possibly changing it in time for 2.10? That'd be good anyways to have two entries in changelog:

# new feature * Add `[python-infer].assets` to infer resources and files from strings # user-api changes * Enable `[python-infer].assets` by default

# 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]

thejcannon · 2022-03-11T14:58:00Z

Updated with:

Asset inference on by default
Reverse mapping of assets by path in a rule
Added file inference. That was the intent from the start, but for whatever reason I didn't test it (and the code wasn't good)

# 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]

Eric-Arellano

Thank you for iterating on this! Looks good once the default is reverted to not break our deprecation policy.

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

This reverts commit d588e39. [ci skip-rust]

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

Eric-Arellano

🎉

benjyw · 2022-03-14T18:56:53Z

Woohoo!!

thejcannon requested review from Eric-Arellano and benjyw January 3, 2022 16:24

thejcannon commented Jan 3, 2022

View reviewed changes

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

thejcannon commented Jan 3, 2022

View reviewed changes

build-support/bin/generate_docs.py Outdated Show resolved Hide resolved

thejcannon commented Jan 3, 2022

View reviewed changes

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

thejcannon commented Jan 3, 2022

View reviewed changes

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

thejcannon force-pushed the infer-resources branch from fc1cfd2 to 39b6655 Compare January 5, 2022 17:57

thejcannon force-pushed the infer-resources branch 2 times, most recently from f124689 to 84e1791 Compare January 7, 2022 02:06

thejcannon commented Jan 7, 2022

View reviewed changes

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

thejcannon commented Jan 7, 2022

View reviewed changes

thejcannon commented Jan 12, 2022

View reviewed changes

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

kaos reviewed Jan 12, 2022

View reviewed changes

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

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

kaos reviewed Jan 12, 2022

View reviewed changes

src/python/pants/backend/python/dependency_inference/scripts/dependency_parser.py Show resolved Hide resolved

thejcannon force-pushed the infer-resources branch 2 times, most recently from 8701b2c to 8bdeb7a Compare January 24, 2022 20:27

thejcannon commented Jan 24, 2022

View reviewed changes

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

Eric-Arellano reviewed Jan 26, 2022

View reviewed changes

thejcannon closed this Jan 27, 2022

thejcannon reopened this Jan 27, 2022

cleanup

06bcac0

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

rm TODO

afa8728

# 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 mentioned this pull request Mar 7, 2022

[internal] Compile string import regex only if it is needed and do it from init to make the script load faster. #14717

Merged

Eric-Arellano previously approved these changes Mar 7, 2022

View reviewed changes

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

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

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

Eric-Arellano requested a review from stuhood March 7, 2022 21:34

Eric-Arellano reviewed Mar 7, 2022

View reviewed changes

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

Eric-Arellano reviewed Mar 7, 2022

View reviewed changes

thejcannon added 5 commits March 8, 2022 19:59

changes

8bd3bac

# 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]

Merge branch 'main' into infer-resources

9f77fb0

changey2

f76d980

# 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]

a little cleanup

68a809f

# 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]

asset

6973a7a

# 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]

Eric-Arellano reviewed Mar 10, 2022

View reviewed changes

thejcannon added 2 commits March 11, 2022 08:46

files too!

209e7ef

# 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]

Assets are on by default

d588e39

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

thejcannon added 2 commits March 11, 2022 08:59

remove debug

8958e02

# 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 name

163e576

# 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]

Eric-Arellano reviewed Mar 12, 2022

View reviewed changes

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

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

thejcannon added 2 commits March 11, 2022 19:39

Revert "Assets are on by default"

3929e8f

This reverts commit d588e39. [ci skip-rust]

comment

fe1802e

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

Eric-Arellano approved these changes Mar 13, 2022

View reviewed changes

thejcannon merged commit b73328f into pantsbuild:main Mar 14, 2022

thejcannon deleted the infer-resources branch March 14, 2022 18:43

Eric-Arellano mentioned this pull request Apr 22, 2022

Go: improve UX of embed and resources #13795

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the ability to infer assets from strings for Python #14049

Add the ability to infer assets from strings for Python #14049

thejcannon commented Jan 3, 2022 •

edited

Loading

thejcannon commented Jan 5, 2022

stuhood commented Jan 5, 2022

thejcannon commented Jan 5, 2022

thejcannon Jan 7, 2022

kaos left a comment

Eric-Arellano left a comment

thejcannon commented Jan 26, 2022

Eric-Arellano commented Jan 26, 2022

thejcannon commented Jan 26, 2022

Eric-Arellano commented Jan 26, 2022

thejcannon commented Jan 27, 2022

Eric-Arellano commented Jan 27, 2022

thejcannon commented Jan 27, 2022

thejcannon commented Mar 3, 2022

Eric-Arellano left a comment

Eric-Arellano left a comment

thejcannon commented Mar 7, 2022

Eric-Arellano Mar 10, 2022

thejcannon Mar 10, 2022

Eric-Arellano Mar 10, 2022

thejcannon Mar 11, 2022

Eric-Arellano Mar 12, 2022

thejcannon commented Mar 11, 2022

Eric-Arellano left a comment

Eric-Arellano left a comment

benjyw commented Mar 14, 2022

		@@ -31,21 +32,37 @@ class ParsedPythonImports(FrozenDict[str, ParsedPythonImportInfo]):
		"""All the discovered imports from a Python source file mapped to the relevant info."""


		class ParsedPythonAssets(DeduplicatedCollection[str]):

Add the ability to infer assets from strings for Python #14049

Add the ability to infer assets from strings for Python #14049

Conversation

thejcannon commented Jan 3, 2022 • edited Loading

High level

thejcannon commented Jan 5, 2022

stuhood commented Jan 5, 2022

thejcannon commented Jan 5, 2022

thejcannon Jan 7, 2022

Choose a reason for hiding this comment

kaos left a comment

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

thejcannon commented Jan 26, 2022

Eric-Arellano commented Jan 26, 2022

thejcannon commented Jan 26, 2022

Eric-Arellano commented Jan 26, 2022

thejcannon commented Jan 27, 2022

Eric-Arellano commented Jan 27, 2022

thejcannon commented Jan 27, 2022

thejcannon commented Mar 3, 2022

Eric-Arellano left a comment

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

thejcannon commented Mar 7, 2022

Eric-Arellano Mar 10, 2022

Choose a reason for hiding this comment

thejcannon Mar 10, 2022

Choose a reason for hiding this comment

Eric-Arellano Mar 10, 2022

Choose a reason for hiding this comment

thejcannon Mar 11, 2022

Choose a reason for hiding this comment

Eric-Arellano Mar 12, 2022

Choose a reason for hiding this comment

thejcannon commented Mar 11, 2022

Eric-Arellano left a comment

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

benjyw commented Mar 14, 2022

thejcannon commented Jan 3, 2022 •

edited

Loading