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 [python-setup].requirement_constraints_target for upcoming Poetry macro #11724

Merged
merged 3 commits into from
Mar 18, 2021

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented Mar 17, 2021

Closes #10723. This adds a new _python_constraints target, which allows defining the constraints as a list of strings in a BUILD file, rather than needing a constraints file on disk. This is useful for macros because they can generate this target.

As decided in that issue, we do not require using this new target - users can continue to use a file on disk via [python-setup].requirement_constraints.

It's not yet clear how this new target type will be used as we add support for multiple lockfiles, so the target is made private for now and its main intended use is macros like Pipenv and Poetry.

[ci skip-rust]
[ci skip-build-wheels]

…ry macro

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

@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.

It's not yet clear how this new target type will be used as we add support for multiple lockfiles, so the target is made private for now and its main intended use is macros like Pipenv and Poetry.

Feedback welcomed if we should make this target type public or not. It's a little weird that the options are public, but target not.

A big reason I leaned towards private is so that _python_constraints doesn't show up with ./pants help targets because it could be confusing if people think they must use this target type, rather than a concrete file. But not sure how important that is.

@jsirois
Copy link
Contributor

jsirois commented Mar 17, 2021

I'm still not understanding why we use macros at all for requirements.txt vs generating field sets in rules from requirements.txt input. Likely, as a result, I'm not a good reviewer for this because I'm ratholed back there.

@Eric-Arellano
Copy link
Contributor Author

Eric-Arellano commented Mar 17, 2021

I'm still not understanding why we use macros at all for requirements.txt vs generating field sets in rules from requirements.txt input.

We still live in a target-centric world, rather than field set-centric world. E.g. we need python_requirement_library targets to work properly with ./pants dependencies. Currently, there is no way for a rule to generate a target ("target gen"?). It can of course, but it won't integrate properly with things like ./pants dependencies.

Perhaps that will change, as CAOFs have major limitations. Iiuc we want to redesign those to use the engine. But for now, I'm trying to unblock a contributor with #10723 and avoiding scope creep of redesigning CAOFs and adding "target gen". The CAOF macros work, even if not very elegant.

Likely, as a result, I'm not a good reviewer for this because I'm ratholed back there.

Sure, feel free to un-add yourself.

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

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

See comment on #7022 re: macros creating multiple targets.

BUT, I think that a constraints.txt file is different from a requirements.txt in one fundamental way: the entries in a constraints file (or any other lockfile) may never be used independently: they are always used together as a single atomic unit (because you don't know until you run the resolve whether an entry in the lockfile is relevant to a particular requirement in the requirements file: it might be transitive).

This might mean that we don't need a macro, because we don't need to create multiple constraints targets per file?

Comment on lines +664 to +673
class PythonRequirementConstraintsField(_RequirementSequenceField):
alias = "constraints"
required = True
help = "A list of pip-style requirement strings, e.g. `my_dist==4.2.1`."


class PythonRequirementConstraints(Target):
alias = "_python_constraints"
core_fields = (*COMMON_TARGET_FIELDS, PythonRequirementConstraintsField)
help = "A private helper target for inlined requirements constraints, used by macros."
Copy link
Member

Choose a reason for hiding this comment

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

The expectation with #11165 is that folks will be creating these manually I think? Also, we'll soon support other types of lockfiles than constraints.txt.

Is it worth skipping to publicly naming this a python_lockfile target (with a constraints= field for the sources)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

with a constraints= field for the sources

We do not want a Sources field here because the main idea of this PR is that you can create constraints purely through strings in a BUILD file - you don't need any file to exist on disk. We already have --requirement-constraints to read a file on disk, it would be repetitive to have the target do it.

However, I had considered removing the --requirement-constraints option to always use the target no matter what. But, we decided to hold off on that given the possible churn for multiple lockfiles.

Also, we'll soon support other types of lockfiles than constraints.txt.
Is it worth skipping to publicly naming this a python_lockfile target

I don't think so. It's not clear yet what these other lockfiles will look like, including if we support multiple formats or only one. I want to avoid premature generalization.

But, this does confirm for me that we probably want to keep this target private.

Copy link
Member

Choose a reason for hiding this comment

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

But, this does confirm for me that we probably want to keep this target private.

If the target is private but the macro is public, you still have "something" that is public. I think that my assumption with making it public is that the macro might not be necessary.

@Eric-Arellano
Copy link
Contributor Author

BUT, I think that a constraints.txt file is different from a requirements.txt in one fundamental way: the entries in a constraints file (or any other lockfile) may never be used independently: they are always used together as a single atomic unit (because you don't know until you run the resolve whether an entry in the lockfile is relevant to a particular requirement in the requirements file: it might be transitive).

Agreed.

This might mean that we don't need a macro, because we don't need to create multiple constraints targets per file?

I don't think so. While it's only creating a single target, we still want a CAOF so that we can automate reading and parsing poetry.lock to create a single target. The # of created targets isn't the interesting part, but rather that automated transformation of poetry.lock into a Sequence[Requirement].

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

I don't want to block this unnecessarily, so #shipit. But:

This might mean that we don't need a macro, because we don't need to create multiple constraints targets per file?

I don't think so. While it's only creating a single target, we still want a CAOF so that we can automate reading and parsing poetry.lock to create a single target. The # of created targets isn't the interesting part, but rather that automated transformation of poetry.lock into a Sequence[Requirement].

It's unclear why that needs to happen via a macro rather than as a codegen step (to turn a field of type ConstraintsFileField into a field of type ConstraintsField, for example). Using a CAOF macro runs afoul of #7022 seemingly unnecessarily.

@Eric-Arellano
Copy link
Contributor Author

It's unclear why that needs to happen via a macro rather than as a codegen step (to turn a field of type ConstraintsFileField into a field of type ConstraintsField, for example)

Yeah that's true, we could have a proper Target that will read poetry.lock and do codegen to convert that into a generated constraints.txt file.

Although, because of the joint requirement with converting pyproject.toml into multiple python_requriement_library targets, we will still for now need a CAOF (until we add synthetic targets). The design sketched in #10655 uses a single macro to generate both the python_requirement_library targets and the constraints file logic, and this PR integrates nicely with that setup.

@Eric-Arellano Eric-Arellano merged commit 3d35d03 into pantsbuild:master Mar 18, 2021
@Eric-Arellano Eric-Arellano deleted the inline-constraints branch March 18, 2021 19:04
Eric-Arellano added a commit that referenced this pull request May 6, 2021
…s_target` option and `_python_constraints` target (#11998)

This was added in #11724 to support a macro for Poetry for constraints files. But we recently decided that the macro will not support automatically reading `Poetry.lock` because it is not very much to ask users to do `poetry --export`. The macro will only operate on `pyproject.toml`. As suggested in https://docs.google.com/document/d/1bCYb0UQZx9a-9tAagydCN_z3826QRvz_3aVnXKSNTJw/edit#heading=h.11w5a92c1zo8, (for now) the lockfile format will look like requirements.txt

This PR's reverted code diverges from the tool lockfile proposal and it's getting in the way.

[ci skip-rust]
[ci skip-build-wheels]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow --python-setup-requirement-constraints to work with inline requirements
3 participants