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

Support pip options for requirements #12090

Closed
GLeurquin opened this issue May 19, 2021 · 5 comments
Closed

Support pip options for requirements #12090

GLeurquin opened this issue May 19, 2021 · 5 comments
Labels
onboarding Issues that affect a new user's onboarding experience

Comments

@GLeurquin
Copy link

I would like to install a pip package with the --no-binary option set.

I tried:

python_requirement_library(name='pydantic', requirements=[
  'pydantic==1.6.1 --no-binary=pydantic'
])

But I get

InvalidFieldException: Invalid requirement 'pydantic==1.6.1 --no-binary=pydantic' in the 'requirements' field for the target 3rdparty/python:pydantic: Parse error at "'--no-bin'": Expected stringEnd

Any way to do this with pants?

@Eric-Arellano Eric-Arellano self-assigned this May 19, 2021
@Eric-Arellano
Copy link
Contributor

Hey @GLeurquin, not yet, but very soon! We're doing a major revamp of third party dependencies as our next big project, including:

  • supporting lockfiles for each tool
  • autogenerating lockfiles for you, rather than that hacky bash script
  • multiple lockfiles possible for your own code

Part of that prework for that project is to implement this issue's feature. FYI https://docs.google.com/document/d/1bCYb0UQZx9a-9tAagydCN_z3826QRvz_3aVnXKSNTJw/edit for the overall project proposal

@GLeurquin
Copy link
Author

Ok, thanks ! For now I'm creating a wheel on a custom repository that is built using the correct flags, then using that as a dependency. Fortunately not too many packages require this, but would indeed be nice to have the option within pants. Thanks for working on it :)

Eric-Arellano added a commit that referenced this issue Jun 8, 2021
…tion messages (#12180)

We already recommend both these options in our docs: https://www.pantsbuild.org/docs/troubleshooting#debug-tip-enable-stack-traces-and-increase-logging. But it's not enough to expect people to find that docs page, we should give the recommendation in the message too.

`--no-process-execution-local-cleanup` will become particularly important with #12090, where we will no longer be able to put the requirements installed by Pex directly in the argv and users will need to inspect the generated requirements.txt file to see what was installed.

Examples:

> (Use --print-stacktrace for more error details and/or --no-process-execution-local-cleanup to inspect chroots and/or -ldebug for more logs. See https://www.pantsbuild.org/v2.6/docs/troubleshooting for common issues. Consider reaching out for help: https://www.pantsbuild.org/v2.6/docs/getting-help.)

> (Use --print-stacktrace for more error details and/or -ldebug for more logs. See https://www.pantsbuild.org/v2.6/docs/troubleshooting for common issues. Consider reaching out for help: https://www.pantsbuild.org/v2.6/docs/getting-help.)

> (See https://www.pantsbuild.org/v2.6/docs/troubleshooting for common issues. Consider reaching out for help: https://www.pantsbuild.org/v2.6/docs/getting-help.)

[ci skip-rust]
[ci skip-build-wheels]
@Eric-Arellano
Copy link
Contributor

@wilsonliam is going to work on this 🎉

See https://pip.pypa.io/en/stable/cli/pip_install/#requirements-file-format for how pip supports options, both global options for the whole requirements file and options for each individual requirement. The below instructions are only for per-requirement options, not global options. @GLeurquin, I'm not sure the below will thus actually solve your issue description. But this can be seen as pre-work for that possible followup. And we must have the --hash support for the upcoming rework of 3rd party requirements.

This can be done in two steps. It probably makes sense for them to be separate PRs, e.g. easier to revert if there are issues.

1: use requirements.txt

Right now, we pass all requirements to install as arguments directly to Pex, e.g. pex Django==1.1 -o foo.pex. The Pex CLI doesn't support Pip arguments.

Instead, we need to use a requirements.txt file and something like pex -r requirements.txt -o foo.pex.

Note, we are not going to use the user's literal requirements.txt file, as requirements can be specified "inline" like with python_requirement_library targets, or the new Poetry macro. Instead, we must generate a requirements.txt file, probably named generated_requirements.txt or requirements.generated.txt. Use await Get(Digest, CreateDigest) to do that.

Specifically, update the build_pex rule to no longer do this:

argv.extend(request.requirements)

Instead, create the digest, merge it in the input_digest, and update the argv to do -r generated_requirements.txt.

I don't think we need to add new tests for this - it's an implementation detail. We only need to make sure the Pex related tests still pass.

2: allow python_requirement_library to have pip args

We currently use pkg_resources.Requirement for the PythonRequirementLibrary target:

class _RequirementSequenceField(Field):
value: tuple[Requirement, ...]
@classmethod
def compute_value(
cls, raw_value: Optional[Iterable[str]], address: Address
) -> Tuple[Requirement, ...]:
value = super().compute_value(raw_value, address)
if value is None:
return ()
invalid_type_error = InvalidFieldTypeException(
address,
cls.alias,
value,
expected_type="an iterable of pip-style requirement strings (e.g. a list)",
)
if isinstance(value, str) or not isinstance(value, collections.abc.Iterable):
raise invalid_type_error
result = []
for v in value:
# We allow passing a pre-parsed `Requirement`. This is intended for macros which might
# have already parsed so that we can avoid parsing multiple times.
if isinstance(v, Requirement):
result.append(v)
elif isinstance(v, str):
try:
parsed = Requirement.parse(v)
except Exception as e:
raise InvalidFieldException(
_format_invalid_requirement_string_error(
v,
e,
description_of_origin=(
f"the '{cls.alias}' field for the target {address}"
),
)
)
result.append(parsed)
else:
raise invalid_type_error
return tuple(result)
class PythonRequirementsField(_RequirementSequenceField):
alias = "requirements"
required = True
help = (
"A sequence of pip-style requirement strings, e.g. `['foo==1.8', "
"\"bar<=3 ; python_version<'3'\"]`."
)

This is really useful because we can do things like get the .project_name for dependency inference:

canonicalize_project_name(req.project_name),

We want to keep using pkg_resources.Requirement, but we also need to capture any additional Pip args. We can use composition to do this (rather than inheritance). Something like:

@dataclass(frozen=True)
class PythonRequirement
    req: pkg_resources.Requirement
    pip_args: tuple[str, ...] = ()

Or probably this would be easier to implement. We don't really need to split the pip args because all we want is to preserve them so that we can put them in the generated requirements.txt. We're not doing anything like looking at individual elements.

@dataclass(frozen=True)
class PythonRequirement
    req: pkg_resources.Requirement
    pip_args: str | None = None

You will want to add an @classmethod parse() that takes a string and creates a PythonRequirement. Similar to your Poetry project, test-driven development is particularly helpful for writing this. You'll probably want to use Python's str.split() method and set maxsplit: https://www.w3schools.com/python/ref_string_split.asp

You will also want to implement __str__ to recombine back into a single string. It'd be good to have a unit test for that.

You'll want to define this in backend/python/target_types.py and then hook it up to PythonRequirementsField. Note that we still want to allow passing either a pre-parsed PythonRequirement (for macros) or a raw string. You'll need to update _RequirementSequenceField, and you can remove that and inline it all into PythonRequirementsField if easier.

To keep things simple, you should be able to keep PexRequirements from pex.py the same for now, that it stores strings rather than PythonRequirements. An advantage of that is that we don't need to update the call sites where we create tool PEXes like for Pylint and Black - they stay the same. We're going to be touching that code a lot to support the overall redesign of 3rdparty requirements, so we can reduce churn for now.

class PexRequirements(DeduplicatedCollection[str]):
sort_input = True
@classmethod
def create_from_requirement_fields(
cls,
fields: Iterable[PythonRequirementsField],
*,
additional_requirements: Iterable[str] = (),
) -> PexRequirements:
field_requirements = {str(python_req) for field in fields for python_req in field.value}
return PexRequirements({*field_requirements, *additional_requirements})

We probably want to add an integration test to pex_test.py. I'm thinking test that --hash works correctly. Choose a requirement like ansicolors that doesn't have any dependencies, and do something like create a simple Poetry project with it and run poetry export to get all the hashes. Copy that entire requirement string with all the hashes, and test that you can correctly build the PEX. Then, in the same test, now use the same requirement but mess up each of the hashes, e.g. change their values by a letter; check that that fails. (We need that failing case to prove that hashes are actually being consumed, that we don't just ignore them. We also want the succeeding case to have positive confirmation things work and that the PEX didn't fail to build for an unrelated reason.)

@benjyw
Copy link
Contributor

benjyw commented Apr 2, 2022

Addressed in #14985

@benjyw
Copy link
Contributor

benjyw commented Apr 5, 2022

Fixed in #14985

@benjyw benjyw closed this as completed Apr 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
onboarding Issues that affect a new user's onboarding experience
Projects
None yet
Development

No branches or pull requests

4 participants