Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to run any PythonSourceField #15849

Merged
merged 34 commits into from
Jun 30, 2022
Merged

Conversation

thejcannon
Copy link
Member

@thejcannon thejcannon commented Jun 16, 2022

(Minor user API change 😄 )

This PR encompasses a few changes which combined achieve the goal of the proposal doc.

It:

  • Adds support for run on a PythonSourceField. The implementation is 100% shifted from running a pex_binary
    • A new field was added to the PythonSourceField targets: run_goal_use_sandbox
    • Unfortunately, for consistency, this (including the new field) applies to _test_utils and _test targets as well 🤷‍♂️
  • Add a global flag --use-deprecated-pex-binary-run-semantics which toggles between the "new" and "old" behavior and must be set (will have the default changed in 2.14 and be removed in 2.15).
  • Special-cases the handling of an ambiguity between 2 targets if it is between a pex_binary and python_source. This allows us a path forward without breaking clients who (very very very often) use ./pants run path/to/file.py to refer to the pex_binary. Which target gets selected is based on --use-deprecated-pex-binary-run-semantics.
  • Removes run_in_sandbox field from pex_binary (this never made it to a stable release, so non-breaking)

[ci skip-rust]
[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]
@thejcannon thejcannon marked this pull request as draft June 16, 2022 18:10
Copy link
Member Author

@thejcannon thejcannon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has a little bit of time-sensitivity due to pulling pex_binary's run_in_sandbox field 😉

src/python/pants/backend/python/goals/run_python_source.py Outdated Show resolved Hide resolved
# @TODO: How should we make this customizable?
# `False` is backwards-compatible behavior right now
run_in_sandbox=False,
# Setting --venv is kosher because the PEX we create is just for the thirdparty deps.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not just kosher, but desirable due to pex-tool/pex#1802. Happy to hear better comment wordsmithing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer necessary if we upgrade PEX to a version which fixes pex-tool/pex#1802

python_sources(name="main_lib",
sources=["main.py"],
# Force-exclude any dep on bar.py, so the only way to consume it is via the dist.
dependencies=[":dist", "!:lib"],
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is slightly tweaked from the pex_binary case because we can't use transitive excludes.

@thejcannon
Copy link
Member Author

thejcannon commented Jun 16, 2022

Can I/should I pull the plug on the in-repo pex_binarys that are just scripts? 😄

EDIT: I'll do this in follow-up PR. I want to be able to demonstrate the ambiguity easily.

# 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]
@thejcannon
Copy link
Member Author

thejcannon commented Jun 17, 2022

Example of deprecations (while there is ambiguity and the pex_binary still exists):

joshuacannon@CEPHANDRIUS:~/work/pants$ ./pants run build-support/bin/generate_docs.py
20:28:54.63 [INFO] Initializing scheduler...
20:28:55.00 [INFO] Scheduler initialized.
20:28:55.27 [WARN] DEPRECATED: referring to a `pex_binary` by using the filename specified in `entry_point` is scheduled to be removed in version 2.14.0dev0.

In Pants 2.14, a `pex_binary` can no longer be referred to by the filename that the `entry_point` field uses.

This is due to a change in Pants 2.13, which allows you to use the `run` goal directly on a `python_source` target without requiring a `pex_binary`. As a consequence Pants removed the ability to refer to the `pex_binary` via its `entry_point`, as otherwise it would be ambiguous which target to use.

Note that because of this change you are able to remove any `pex_binary` targets you have declared just to support the `run` goal (usually these are developer scripts), as using `run` with the filename will have the equivalent behavior.

To fix this deprecation, use the `pex_binary`'s address.
20:28:55.30 [WARN] DEPRECATED: the option --pex-run-packaged-firstparty-code defaulting to false is scheduled to be removed in version 2.14.0dev0.

In Pants 2.14, running a `pex_binary` will actually package the PEX and run it, as if you ran `package` followed by executing the built PEX.

To fix this deprecation, explictly set `run_packaged_firstparty_code` in the `[pex]` section of `pants.toml`. Set it to `false` to use the "old" behavior. Set it to `true` to use the "new" behavior.

# 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]
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay!!

Docs need to be updated for the Python package and run pages before merging. I'm gonna be OOO till Thursday, but lmk if you want an assist when I get back.

Our deprecation messages might want to suggest for people that they get rid of pex_binary targets they use to have. No need. And you can delete all of build-support/bin I think :)

# 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
Copy link
Member Author

Yay!!

Docs need to be updated for the Python package and run pages before merging. I'm gonna be OOO till Thursday, but lmk if you want an assist when I get back.

Our deprecation messages might want to suggest for people that they get rid of pex_binary targets they use to have. No need. And you can delete all of build-support/bin I think :)

I'll take a stab at the docs.

The deprecation message does make the suggestion:

Note that because of this change you are able to remove any pex_binary targets you have declared just to support the run goal (usually these are developer scripts), as using run on the python_source will have the equivalent behavior.

I'll dedicate a separate PR for deletion of our binary targets 😄

@benjyw
Copy link
Contributor

benjyw commented Jun 21, 2022

| My vote is well-documented flag on run (and eventually repl)

An issue with that is that it's meaningless for compiled languages.

@thejcannon
Copy link
Member Author

thejcannon commented Jun 21, 2022

| My vote is well-documented flag on run (and eventually repl)

An issue with that is that it's meaningless for compiled languages.

That's the well documented part 😉 This is relevant for any language which can run on the loose files, so mostly scripting languages.

Maybe I just make it a field, and hope either the default covers most cases or its rare people will want to toggle between the two modes on the same target easily?

@benjyw
Copy link
Contributor

benjyw commented Jun 21, 2022

Naming nit: --run-packaged-firstparty-code is ungainly. How about --package (So it's --run-as-package in global position or ./pants run --as-package in goal position.

@benjyw
Copy link
Contributor

benjyw commented Jun 21, 2022

I'm still trying to figure out if I think this is intuitive to Winston. I think on balance yes? "Run a source file" seems to intuitively mean "run it and its deps directly from source" and "Run a package" seems to intuitively mean "build an executable and run it". I think the risks re --changed and --loop not doing the right thing are acceptable.

@thejcannon
Copy link
Member Author

Naming nit: --run-packaged-firstparty-code is ungainly. How about --package (So it's --run-as-package in global position or ./pants run --as-package in goal position.

I like the idea, but the option is on the pex subsystem 😢

# 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
Copy link
Member Author

I think the risks re --changed and --loop not doing the right thing are acceptable.

They do the right thing, just on the metadata they have (which might not be right). It's a slight difference, but an important one IMO.

Because the use-case we're explicitly not considering isn't that it "doesnt work", but that run shouldn't be used to validate your metadata.

@thejcannon thejcannon marked this pull request as ready for review June 22, 2022 00:54
Comment on lines +165 to +167
were packaged in a `pex_binary`. Additionally, it may be necessary if your sources depend
transitively on "generated" files which will be materialized in the sandbox in a source
root, but are not in-repo.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya know, we can technically inspect the dep tree for this and issue a warning if the field isn't explictly set or something similar 🤔

Not saying we should block this PR on it, but worth an issue filed once this is in.

# 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
Copy link
Member Author

  • Docs are added
  • Field has been added as well
  • No longer draft PR

…to runpython

# 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
Copy link
Member Author

Naming nit: --run-packaged-firstparty-code is ungainly. How about --package (So it's --run-as-package in global position or ./pants run --as-package in goal position.

I like the idea, but the option is on the pex subsystem cry

Also FWIW this will likely be set in pants.toml and will be deprecated/removed after 2 releases. So kinda like the use_old_style_macros (or whatever it was called)

@thejcannon
Copy link
Member Author

Hmmm this just occurred to me, how should we handle restartable? Why is that a field and not a run flag?

# 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]
# 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]
# 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]
thejcannon added a commit to thejcannon/pants that referenced this pull request Jun 28, 2022
run's counterpart to pantsbuild#15799. Support is added for pex_binary (which will be spiritually lifted to the python_source with pantsbuild#15849)

[ci skip-rust]
[ci skip-build-wheels]
thejcannon added a commit to thejcannon/pants that referenced this pull request Jun 28, 2022
run's counterpart to pantsbuild#15799. Support is added for pex_binary (which will be spiritually lifted to the python_source with pantsbuild#15849)

[ci skip-rust]
[ci skip-build-wheels]
stuhood pushed a commit that referenced this pull request Jun 29, 2022
`run`'s counterpart to #15799. Support is added for `pex_binary` (which will be spiritually lifted to the `python_source` with #15849)

[ci skip-rust]
[ci skip-build-wheels]
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huzzah!

docs/markdown/Python/python-goals/python-run-goal.md Outdated Show resolved Hide resolved
docs/markdown/Python/python-goals/python-run-goal.md Outdated Show resolved Hide resolved
docs/markdown/Python/python-goals/python-run-goal.md Outdated Show resolved Hide resolved
docs/markdown/Python/python-goals/python-run-goal.md Outdated Show resolved Hide resolved
src/python/pants/backend/python/target_types.py Outdated Show resolved Hide resolved
src/python/pants/option/global_options.py Show resolved Hide resolved
src/python/pants/option/global_options.py Outdated Show resolved Hide resolved
thejcannon and others added 5 commits June 29, 2022 19:08
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.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]
…to runpython

# 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]
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@thejcannon thejcannon merged commit 27628c3 into pantsbuild:main Jun 30, 2022
@thejcannon thejcannon deleted the runpython branch June 30, 2022 01:23
Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request Jun 30, 2022
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
Eric-Arellano pushed a commit that referenced this pull request Jul 1, 2022
With #15849 the default for the `--tailor-pex-binary-targets` flag should naturally be `False`, so this begins the path to that.

[ci skip-rust]
[ci skip-build-wheels]
thejcannon added a commit to thejcannon/pants that referenced this pull request Jul 1, 2022
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>

[ci skip-build-wheels]
thejcannon added a commit to thejcannon/pants that referenced this pull request Jul 1, 2022
…ld#15962)

With pantsbuild#15849 the default for the `--tailor-pex-binary-targets` flag should naturally be `False`, so this begins the path to that.

[ci skip-rust]
[ci skip-build-wheels]
Eric-Arellano pushed a commit that referenced this pull request Jul 1, 2022
…16022)

(Minor user API change 😄 )

This PR encompasses a few changes which combined achieve the goal of [the proposal doc](https://docs.google.com/document/d/1R9m-DtM8Y5UKx5iam5ipeK531tZR40_P7El4ucYq4Po).

It:
- Adds support for `run` on a `PythonSourceField`. The implementation is 100% shifted from `run`ning a `pex_binary`
   - A new field was added to the `PythonSourceField` targets: `run_goal_use_sandbox`
   - Unfortunately, for consistency, this (including the new field) applies to `_test_utils` and `_test` targets as well 🤷‍♂️ 
- Add a global flag `--use-deprecated-pex-binary-run-semantics` which toggles between the "new" and "old" behavior and must be set (will have the default changed in 2.14 and be removed in 2.15).
- Special-cases the handling of an ambiguity between 2 targets if it is between a `pex_binary` and `python_source`. This allows us a path forward without breaking clients who (very very very often) use `./pants run path/to/file.py` to refer to the `pex_binary`. Which target gets selected is based on `--use-deprecated-pex-binary-run-semantics`.
- Removes  `run_in_sandbox` field from `pex_binary` (this never made it to a stable release, so non-breaking)


[ci skip-rust]
[ci skip-build-wheels]
Eric-Arellano pushed a commit that referenced this pull request Jul 1, 2022
…ick of #15962) (#16023)

With #15849 the default for the `--tailor-pex-binary-targets` flag should naturally be `False`, so this begins the path to that.

[ci skip-rust]
[ci skip-build-wheels]
thejcannon added a commit that referenced this pull request Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants