Seems to work, e.g. https://github.com/pantsbuild/pants/pull/19255

Describe the bug

The new Rust parser doesn't seem to understand a 'plain' parent import, like from ..file1 import *. It does understand from ..file1 import func, though.

This reproducer constructs a directory structure like:

.
├── pants.toml
└── src
    └── a
        ├── BUILD
        ├── file1.py
        └── file2.py

where file2.py has from .file1 import *. It runs pants peek src/a/file2.py with both the python parser (which shows a dependency on src/a/file1.py) and with the Rust parser (which doesn't). Unlike #19173, there's no inference warnings either.

cd $(mktemp -d)

cat > pants.toml <<EOF
[GLOBAL]
pants_version = "2.17.0rc0"

backend_packages = ["pants.backend.python"]

[python]
interpreter_constraints = [">=3.8"]

[python-infer]
use_rust_parser = false
EOF

# set up the file structure:
mkdir -p src/a
touch src/a/file1.py
echo "from .file1 import *" > src/a/file2.py

echo "python_sources()" > src/a/BUILD

tree .

# OK: with Python parser `"dependencies": ["src/a/file1.py"]`
pants peek src/a/file2.py

# flip to the rust parser
sed -i '' 's/use_rust_parser = false/use_rust_parser = true/' pants.toml

# BUG: with Rust parser `"dependencies": []`
pants peek src/a/file2.py

Pants version 2.17.0rc0

OS macOS

Additional info #19173

Describe the bug

The new Rust parser doesn't seem to understand a 'plain' parent import, like from ..file1 import *. It does understand from ..file1 import func, though.

This reproducer constructs a directory structure like:

.
├── pants.toml
└── src
    └── a
        ├── BUILD
        ├── file1.py
        └── file2.py

where file2.py has from .file1 import *. It runs pants peek src/a/file2.py with both the python parser (which shows a dependency on src/a/file1.py) and with the Rust parser (which doesn't). Unlike #19173, there's no inference warnings either.

cd $(mktemp -d)

cat > pants.toml <<EOF
[GLOBAL]
pants_version = "2.17.0rc0"

backend_packages = ["pants.backend.python"]

[python]
interpreter_constraints = [">=3.8"]

[python-infer]
use_rust_parser = false
EOF

# set up the file structure:
mkdir -p src/a
touch src/a/file1.py
echo "from .file1 import *" > src/a/file2.py

echo "python_sources()" > src/a/BUILD

tree .

# OK: with Python parser `"dependencies": ["src/a/file1.py"]`
pants peek src/a/file2.py

# flip to the rust parser
sed -i '' 's/use_rust_parser = false/use_rust_parser = true/' pants.toml

# BUG: with Rust parser `"dependencies": []`
pants peek src/a/file2.py

Pants version 2.17.0rc0

OS macOS

Additional info #19173

This fixes #19248 by implementing explicit handling for * wildcard imports in the Rust-based Python dependency inference parser.

The commits are somewhat individually reviewable:

1. switching how the insert_import arguments are consumed from the syntax tree, for from ... import ... statements in particular:
   • before: name and module_name correspond to import $name, "$name", from $module_name import $name
   • after: base and imported correspond to import $base, "$base", from $base import $imported
   • This is in particular flipping the last one, and changes whether the from ... part is the optional arg (before) or not (after).
   • The motivation is that there's more functional similarities between the from ... part and a plain import ... statement, than between the import ... part and a plain import ... statement, despite the superficial similarities of it! (from foo import bar is a little like import foo as __secret; bar = __secret.bar.)
2. Add some tests that fail
3. Fix the bug
4. (and others) some tweaks, including defence-in-depth against similar problems happening in future

This adjusts the cherry picking to create the PRs with a milestone attached, so that they're surfaced in the milestone views like https://github.com/pantsbuild/pants/milestone/58, alongside any pending issues and not-yet-cherrypicked PRs.

In particular, if a PR is going to be cherry-picked, it'll have the milestone attached and be visible in that view (and any issues it's fixing may also have it tagged)... but once PR merges to main, it and any associated issues will be closed. This means they disappear from the milestone view, but there's still work remaining to actually fix them for that milestone: landing the cherry-pick. Pending cherry-pick PRs can be discovered by doing a search like base:2.17.x, but it's nice to have only one place to check (the milestone view) rather than needing to remember to also construct that search manually.

This is a response to https://github.com/pantsbuild/pants/pull/19110 sitting around for a while and delaying 2.16, due to flaky CI failures: it was hard to notice.

See also https://pantsbuild.slack.com/archives/C0D7TNJHL/p1685929902870459 / https://chat.pantsbuild.org/t/12317552/looks-like-https-github-com-pantsbuild-pants-pull-19110-neve#4a387fb1-fc0c-4395-9160-74adf8ab7944

(This will cause some conflicts with #19230, but this PR is a quick win that's easily ported over.)

This adjusts the cherry picking to create the PRs with a milestone attached, so that they're surfaced in the milestone views like https://github.com/pantsbuild/pants/milestone/58, alongside any pending issues and not-yet-cherrypicked PRs.

In particular, if a PR is going to be cherry-picked, it'll have the milestone attached and be visible in that view (and any issues it's fixing may also have it tagged)... but once PR merges to main, it and any associated issues will be closed. This means they disappear from the milestone view, but there's still work remaining to actually fix them for that milestone: landing the cherry-pick. Pending cherry-pick PRs can be discovered by doing a search like base:2.17.x, but it's nice to have only one place to check (the milestone view) rather than needing to remember to also construct that search manually.

This is a response to https://github.com/pantsbuild/pants/pull/19110 sitting around for a while and delaying 2.16, due to flaky CI failures: it was hard to notice.

See also https://pantsbuild.slack.com/archives/C0D7TNJHL/p1685929902870459 / https://chat.pantsbuild.org/t/12317552/looks-like-https-github-com-pantsbuild-pants-pull-19110-neve#4a387fb1-fc0c-4395-9160-74adf8ab7944

(This will cause some conflicts with #19230, but is a quick win that's easily ported over.)

This changes the behaviour of the runtime field of the python_aws_lambda_function, python_aws_lambda_layer and python_google_cloud_function FaaS targets to be translated into a Pants-provided complete platform (if known), rather than using the less specific --platform argument.

For example, when building a target like python_aws_lambda_function(..., runtime="python3.10"), Pants will pull out an appropriate complete platform resources and use that when building the Lambda function to choose the correct wheels.

This is motivated by:

1. the FaaS runtimes generally being stable (AIUI), so it's actually reasonable/possible to provide this
2. the naive wheel selection for --platform often fails and users have to switch to using complete platforms themselves, and, one can still use complete_platforms manually, so this is not worse
3. the switch to layout = "zip" means that passing both runtime="..." and complete_platforms=[...] will fail, as one --platform=... or --complete-platform=... argument can be passed to the underlying PEX call

The moving parts are:

1. a script build-support/bin/generate_faas_complete_platforms.py that reads information about each FaaS system, including runtime docker repo and the tags for each 'known runtime', and shells out to docker to extra a complete platform JSON file
2. if complete_platforms isn't specified, convert the runtime a complete platform argument (if it's known), or continue falling back to the naive --platform style

Notes:

• Notable failing: this is only implemented for AWS: I cannot find an equivalent to https://gallery.ecr.aws/lambda/python for GCF, so despite being generic infrastructure, there's no 'known' runtimes for GCF.
  • Does anyone know of docker images that simulate the runtime environment of GCF?
  • (I've found https://github.com/GoogleCloudPlatform/python-runtime but it's for AppEngine and is quite old, and https://cloud.google.com/functions/docs/concepts/python-runtime is very cagey about Python patch versions, let alone GLibc/manylinux tags!)
• This changes the interaction between the runtime and complete_platforms field:
  • passing both is deprecated, and the behaviour changes too (runtime is now totally ignored, but, per point 3 of the motivation above, this would fail anyway)
  • passing complete_platforms=[] is now permitted, and, I hope, means the PEX calls will use the ambient interpreter, e.g. if running in a compatible docker image
• I haven't written explicit tests for the inference behaviour yet, as I didn't want to go too deep down that path before validating that this is sensible
• I've only implemented this for layout = "zip", as layout = "lambdex" will be removed soon.
• (Once we have this, I think we can even do one step better: infer an appropriate runtime from interpreter constraints (e.g. if a repo has constraints that cover only one major version, like ==3.10.*, infer runtime="python3.10"), so that a minimal FaaS target can be python_aws_lambda_function(name="foo", handler="./foo.py:handler").)

Fixes #18195

I've pushed a few changes, including a defensive one to hopefully make problems like this more obvious in future: make sure there's always some sort of import inserted every time a from .. import .. statement is visited, because inserting nothing is "obviously" wrong. In particular, it's particularly dangerous that inference failed silently here, and I only identified it because tests failed when running 2.17.0rc0 in our CI, rather than getting noisy dep inference failure warnings when running simple commands locally.

You're going to get bit by https://github.com/pantsbuild/pants/issues/18961 if you haven't already.

Thanks, I got bit by that for testing #19173 😅

(On that note, I have a suspicion that --no-local-cache might not work for this? Even with that flag I think I was getting cached results)

I was thinking of computing a hash of the files during the build step and using that. Otherwise we need to manually bump the (not-yet-added) version.

Thoughts?

Either seems fine to me, both have trade-offs:

• manual version: 'precise' (e.g. doesn't change for no-functionality-change edits, like reformatting or adding comments), but potential to forget to change
• hash: potentially awkward to correctly hash the right things (all relevant files and all dependency versions, including down to tree sitter (relates to #8445)), and changes spuriously more often

On balance, I guess hashing is probably better for now: this runs fast enough that (for most users) rerunning when upgrading pants versions is probably unnoticeable compared to all the other work that happens?

I would personally consider turning off this alignment completely: it means adding or removing a long option is more likely to lead to spurious diffs on other lines, due to needing to adjust their alignment.

(I'm also not silly enough to dive down a syntax bikeshed, though: having auto formatting in some form is better than getting mired in a config argument!)

This fixes #19248 by implementing explicit handling for * wildcard imports in the Rust-based Python dependency inference parser.

The commits are individually reviewable:

1. switching how the insert_import arguments are consumed from the syntax tree, for from ... import ... statements in particular:
   • before: name and module_name correspond to import $name, "$name", from $module_name import $name
   • after: base and imported correspond to import $base, "$base", from $base import $imported
   • This is in particular changing whether the from ... part is the optional (before) or not (after).
   • The motivation is that there's more functional similarities between the from ... part and a plain import ... statement, than between the import ... part and a plain import ... statement, despite the superficial similarities of it! (from foo import bar is a little like import foo as __secret; bar = __secret.bar.)
2. Add some tests that fail
3. Fix the bug

Describe the bug

The new Rust parser doesn't seem to understand a 'plain' parent import, like from ..file1 import *. It does understand from ..file1 import func, though.

This reproducer constructs a directory structure like:

.
├── pants.toml
└── src
    └── a
        ├── BUILD
        ├── file1.py
        └── file2.py

where file2.py has from .file1 import *. It runs pants peek src/a/file2.py with both the python parser (which shows a dependency on src/a/file1.py) and with the Rust parser (which doesn't):

cd $(mktemp -d)

cat > pants.toml <<EOF
[GLOBAL]
pants_version = "2.17.0rc0"

backend_packages = ["pants.backend.python"]

[python]
interpreter_constraints = [">=3.8"]

[python-infer]
use_rust_parser = false
EOF

# set up the file structure:
mkdir -p src/a
touch src/a/file1.py
echo "from .file1 import *" > src/a/file2.py

echo "python_sources()" > src/a/BUILD

tree .

# OK: with Python parser `"dependencies": ["src/a/file1.py"]`
pants peek src/a/file2.py

# flip to the rust parser
sed -i '' 's/use_rust_parser = false/use_rust_parser = true/' pants.toml

# BUG: with Rust parser `"dependencies": []`
pants peek src/a/file2.py

Pants version 2.17.0rc0

OS macOS

Additional info #19173

FWIW my second act was going to be a new GHA check (extending the label check) to check for a milestone if there's a CherryPick label

👍

Even with that initial checker (which sounds good), I think it'd be good to have the script still handle the no milestone case. In particular, it might be easy for the script to 'succeed' at doing nothing, if a PR ended up with no milestone. This might mean it clears out the "needs-cherrypick" label without actually doing any cherry picking.

For instance, this sequence seems plausible (maybe not with the current code, but maybe with a small tweak in future):

• someone adds a milestone before CI runs and it passes, but it's removed by the time the PR merged (either accidentally or on purpose)
• cherry picker runs, and ends up with an empty set of milestones
• the "do the appropriate PRs exist" check is trivially true (there's no milestones, so having no PRs is correct ✅ ), and so the script falls through to removing the label

Ah, yeah, that'd work, as long as we're good about always cherry-picking any changes.

Describe the bug

The new Rust parser doesn't seem to understand a 'plain' parent import, like from .. import foo. It does understand from ..foo import bar, though.

For instance, the following reproducer sets up a file tree like the following, with file2 containing from .. import file1.

.
├── pants.toml
└── src
    └── a
        ├── BUILD
        ├── b
        │   ├── BUILD
        │   └── file2.py
        └── file1.py

Running it with the Rust parser prints the following warning, and indeed, the peek output doesn't include a dependency from file2 to file1:

20:32:17.51 [WARN] Pants cannot infer owners for the following imports in the target src/a/b/file2.py:

  * ...file1 (line: 1)

If you do not expect an import to be inferrable, add `# pants: no-infer-dep` to the import line. Otherwise, see https://www.pantsbuild.org/v2.17/docs/troubleshooting#import-errors-and-missing-dependencies for common problems.

Reproducer:

cd $(mktemp -d)

cat > pants.toml <<EOF
[GLOBAL]
pants_version = "2.17.0a1"

backend_packages = [
  "pants.backend.python",
]

[python]
interpreter_constraints = [">=3.8"]

[python-infer]
use_rust_parser = false
EOF

# set up the nested file structure:
mkdir -p src/a/b
touch src/a/file1.py
echo "python_sources()" > src/a/BUILD

echo "from .. import file1" > src/a/b/file2.py
echo "python_sources()" > src/a/b/BUILD

# confirm the layout
tree .

# OK: with Python parser `"dependencies": ["src/a/file1.py"]`
pants peek src/a/b/file2.py

# flip to the rust parser
sed -i '' 's/use_rust_parser = false/use_rust_parser = true/' pants.toml

# BUG: with Rust parser `"dependencies": []`
pants peek src/a/b/file2.py
# "[WARN] Pants cannot infer owners for the following imports in the target src/a/b/file2.py:"

Pants version 2.17.0a1

OS macOS

Additional info N/A