Support installing requirements from inline script metadata (PEP 723)

[SnoopJ]

This changeset adds support for installing requirements declared in a script using inline metadata as defined by PEP 723

I'm sure this still needs changes, but I'm at the point where I'm ready for a maintainer to point out to me how much I've missed 😅

Closes #12891

[notatallshaw]

Hi @SnoopJ thanks for submitting this PR. Let us know if you are willing to continue working on it, I will be willing to review this (although be aware it may be a couple of months, or more, before I can spend time doing a full review).

If so the first thing you need to do is merge, or rebase, with main, resolve any conflicts, and add this additional option to the lock command, which was probably added after you raised this PR. Once you've done that I can advise on any pre-commit failures.

Finally, I don't want to get into a long bike-shedding discussion, but I don't love "--script" as an option name, to me it could be confused with running a script, perhaps "--script-dependencies" or "--script-inline"? Something that makes it clear that just the inline dependencies are being taken from the script.

[SnoopJ]

Hi @SnoopJ thanks for submitting this PR. Let us know if you are willing to continue working on it, I will be willing to review this (although be aware it may be a couple of months, or more, before I can spend time doing a full review).

Yep, I am willing to resume work on this. Thanks for having a look, it sounds like I'm on the right-enough track that I can freshen this PR and fix up whatever was failing CI when this fell fallow.

Finally, I don't want to get into a long bike-shedding discussion, but I don't love "--script" as an option name, to me it could be confused with running a script, perhaps "--script-dependencies" or "--script-inline"? Something that makes it clear that just the inline dependencies are being taken from the script.

Strictly speaking, this implementation isn't "just" the inline dependencies, since it also respects requires-python, so I'm inclined towards the second option since it is describing a broader set of behavior.

--script-inline feels a little opaque to me from the user's perspective, but I'll have a think and see if any other options occur to me while I fix the more obvious problems with this PR.

[pfmoore]

I agree with @notatallshaw that --script is too confusing as an option name. I quite like --requirements-from-script. It's a bit long, but it's very clear and explicit. I don't like --script-inline, as you say it's a bit opaque from a user POV. I could live with --script-dependencies, but it obscures the fact that it checks requires-python (presumably it just errors out if the interpreter is an incompatible version?)

[SnoopJ]

presumably it just errors out if the interpreter is an incompatible version?

Yep!

[ichard26]

First pass review. I'll have to think about the option naming some more, but overall, I'm supportive of the feature. While I do agree this is a suboptimal use of inline script metadata, if pip is meant to be a standards package installer, it should be able to install requirements declared in any standardized format.

[ichard26]

Oh, and thank you for filing a PR! I'm sorry that we've been slow to review.

[SnoopJ]

Oh, and thank you for filing a PR! I'm sorry that we've been slow to review.

Thanks for the review @ichard26. No worries, it was made clear to me before my last pass that review was going to take a while yet (I had planned to do a nudge in January 😜)

I very much appreciate the pointers to the "new/right" way to do things in the test suite, I like the new tests much more after accounting for that feedback.

Edit: looks like I may have caused a signoff problem with marking 003a443 as Co-authored-by, let me know if the history needs rewriting to deal with that.

[ichard26]

The tests are much nicer, indeed!

Other than my remaining inline feedback, my last concern is the inclusion of the -s short option. I don't have particularly well-defined reasons for why I'm hesitant to include it other than that I don't want pip install -s to become popular, especially since it's not the most clear with what it does.

[pfmoore]

Other than my remaining inline feedback, my last concern is the inclusion of the -s short option.

I agree. I think we can reasonably omit the -s option for now, and include it later if there's sufficient demand for it. It's convenient, but I agree that it's not obvious.

[SnoopJ]

Thanks, made those changes.

I also added two new tests for the failure modes, which turned out to be informative because it turns out that the reference implementation has surprising match behavior for multiple inline metadata blocks unless there is a line that does not start with \s*# separating the two. If they aren't separated like that, it's all collated into one match, no matter how many /// TYPE blocks there are inside of that match. This probably makes more sense with a code example:

click for code

import re

script_A = """
# /// script
# data (1)
# ///
#
# /// script
# data (2)
# ///
"""

script_B = """
# /// script
# data (1)
# ///

# /// script
# data (2)
# ///
"""

# These lines adapted from PEP 723's reference parser:
# https://peps.python.org/pep-0723/#reference-implementation

REGEX = r"(?m)^# /// (?P<type>[a-zA-Z0-9-]+)$\s(?P<content>(^#(| .*)$\s)+)^# ///$"
name = "script"
matches_A = list(
    filter(lambda m: m.group("type") == name, re.finditer(REGEX, script_A))
)
matches_B = list(
    filter(lambda m: m.group("type") == name, re.finditer(REGEX, script_B))
)

# output:
# 1
# 2
print(len(matches_A))
print(len(matches_B))

The upshot is that the parser used here would still fail when calling loads() on the match (because the interior /// lines are invalid TOML), which meets PEP 723's MUST error requirement. But I thought I'd call it out in case it's cause enough to modify the reference implementation to detail with this edge case.

EDIT: changing the pattern's last + to +? to make the match non-greedy seems to fix this edge case, so maybe the change is simple.

[ichard26]

Edit: looks like I may have caused a signoff problem with marking 003a443 as Co-authored-by, let me know if the history needs rewriting to deal with that.

I'll squash merge this PR, so it won't matter. Don't worry about it!

EDIT: changing the pattern's last + to +? to make the match non-greedy seems to fix this edge case, so maybe the change is simple.

Hmm, as much as I would like to ensure error message accuracy, I am wary of deviating from the reference implementation. We may just be swapping one edge case for another, but at least we can hand-wave the current one away as "it's what the reference implementation does." I wonder if this has been flagged to the PEP 723 authors yet.

Thanks a lot for bearing with my nit-picky reviews! I'll wait for @notatallshaw to review as he did self-request a review, but from my end, this looks great!

[pfmoore]

From the PEP:

Precedence for an ending line # /// is given when the next line is not a valid embedded content line as described above. For example, the following is a single fully valid block:

# /// some-toml
# embedded-csharp = """
# /// <summary>
# /// text
# ///
# /// </summary>
# public class MyClass { }
# """
# ///

I'm not sure I understand the text part of that quote, but the example seems clear that the reference implementation is behaving correctly.

[notatallshaw]

The upshot is that the parser used here would still fail when calling loads() on the match (because the interior /// lines are invalid TOML), which meets PEP 723's MUST error requirement. But I thought I'd call it out in case it's cause enough to modify the reference implementation to detail with this edge case.

EDIT: changing the pattern's last + to +? to make the match non-greedy seems to fix this edge case, so maybe the change is simple.

Let's keep to the reference implementation, a similar discussion about tweaking the reference implementation came up with uv: astral-sh/uv#10918. I lean on the side of users should not expect things to work they wouldn't work against the reference implementation.

If someone wants to update the spec in the future it's a lot easier to go from error to working, than working to error.

[SnoopJ]

I agree about sticking to the reference implementation, especially as the edge-case is already so small since there's only one TYPE currently recognized by the official standards.

I've filed an issue with the PyPA specs for broader discussion.

[notatallshaw]

Looks good to me.

[ichard26]

Congratulations and thank you for your tenacity in getting this over the line @SnoopJ !