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

Improve export goal to handle multiple Python resolves #14436

Merged
merged 6 commits into from
Feb 11, 2022

Conversation

Eric-Arellano
Copy link
Contributor

We generate a distinct virtualenv for each resolve, using the path dist/python/virtualenvs/<resolve>. From there, the user can load in their IDE which one to use.

If users want to only generate for one IDE, they can use the peek snippet from #14327.

--

If enable_resolves = false, we stick with the old behavior. This is convenient because it would be a misnomer to write the path to [python].default_resolve, which doesn't make sense to use if resolves aren't enabled.

We log a warning when you do migrate to enable-resolves that the old path will no longer be valid.

This allows us to partition, e.g. one venv per resolve.

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

@benjyw benjyw left a comment

Choose a reason for hiding this comment

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

Just some naming nits.

@@ -51,7 +52,7 @@ class Symlink:

@frozen_after_init
@dataclass(unsafe_hash=True)
class ExportableData:
class ExportResult:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the renaming? It makes this harder to review, and it doesn't seem to achieve any purpose beyond preference. If anything, the new naming is less coherent, since "ExportRequest" sounds like a request to perform exporting, whereas it really is a request for data that the caller then actually exports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because Data is awkward with plural. I didn't want to say Datas, nor Datum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open to other suggestions. I thought about Entity and Thing...Data doesn't seem it tho

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that ExportResult(s) aligns with FmtResult(s), LintResult(s), and CheckResult(s).

exportables = await MultiGet(
Get(ExportableData, ExportableDataRequest, request) for request in requests
)
all_results = await MultiGet(Get(ExportResults, ExportRequest, request) for request in requests)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment re renaming throughout, it just makes the salient changes harder to review.

@dataclass(frozen=True)
class _ExportVenvRequest(EngineAwareParameter):
resolve: str | None
root_python_targets: tuple[Target, ...]
Copy link
Contributor

Choose a reason for hiding this comment

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

The word "root" here seems to be more confusing than useful? AFAICT there is nothing here that requires these to be "root targets" in the usual sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're the input targets that you specified to run on, as compared to their dependencies. This change is in line with #14323

@Eric-Arellano Eric-Arellano merged commit 793a39e into pantsbuild:main Feb 11, 2022
@Eric-Arellano Eric-Arellano deleted the export-multiple-resolves branch February 11, 2022 20:19
Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request Feb 11, 2022
…14436)

We generate a distinct virtualenv for each resolve, using the path `dist/python/virtualenvs/<resolve>`. From there, the user can load in their IDE which one to use.

If users want to only generate for one IDE, they can use the `peek` snippet from pantsbuild#14327.

--

If `enable_resolves = false`, we stick with the old behavior. This is convenient because it would be a misnomer to write the path to `[python].default_resolve`, which doesn't make sense to use if resolves aren't enabled.

We log a warning when you do migrate to `enable-resolves` that the old path will no longer be valid.

[ci skip-rust]
# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
Eric-Arellano added a commit that referenced this pull request Feb 11, 2022
… of #14436) (#14454)

We generate a distinct virtualenv for each resolve, using the path `dist/python/virtualenvs/<resolve>`. From there, the user can load in their IDE which one to use.

If users want to only generate for one IDE, they can use the `peek` snippet from #14327.

--

If `enable_resolves = false`, we stick with the old behavior. This is convenient because it would be a misnomer to write the path to `[python].default_resolve`, which doesn't make sense to use if resolves aren't enabled.

We log a warning when you do migrate to `enable-resolves` that the old path will no longer be valid.

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

2 participants