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 Pex's --path-mapping with lockfiles for better local requirement support #16584

Merged
merged 6 commits into from
Aug 24, 2022

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented Aug 19, 2022

Closes #16416.

As explained in the new docs, some users will be able to use fixed paths, which is the simplest approach. But others may have different paths per machine, e.g. using /User/<username> in the path. --path-mappings allows lockfile generators & consumers to set this on a per-machine basis.

Tip to set to a common value

If you can use a common location and Pants's interpolation, e.g. %(buildroot)s/wheels_dir, then the config only needs to be set once. So, we encourage this approach.

This is deemed adequate compared to some other enhancements we could make:

  • Trying to auto-detect when we can derive a common value.
  • Eagerly erroring if users haven't appropriately configured [python-repos].path_mapping.

Global option vs. per-resolve

The design doc at https://docs.google.com/document/d/1HAvpSNvNAHreFfvTAXavZGka-A3WWvPuH0sMjGUCo48/edit proposed having all resolve-related config be configurable on a per-resolve basis, rather than globally. We implemented this idea for a few options, but decided to not implement it (yet?) for [python-repos].{find_links,indexes} #16530.

Given that it is only possible to have a global setting for [python-repos].find_links, I could not come up with a compelling reason to let you have path mappings be per-resolve. It only complicated the user interface and help message because users have to think about resolves.

If we do end up implementing #16530, then it would make sense to also have this option be per-resolve. Until then, there is little value.

@Eric-Arellano
Copy link
Contributor Author

This still needs tests, but otherwise should be ready. cc @jsirois @benjyw if you have feedback on the design direction.

@jsirois
Copy link
Contributor

jsirois commented Aug 19, 2022

I did not go with this approach because it seemed difficult to deduce when a user wants to use a fixed path amongst all users vs. using path mappings.

I was thinking the algorithm was auto-map if the abs path lies within the build root. I cannot see any situation where you'd not mean that path to be relative to the build root and mapped. The only reason I didn't do this in Pex itself is because there is no natural home base except PWD and that seemed potentially surprising, not what you want. Pants though has a well defined home base - the root of the repo.

@Eric-Arellano
Copy link
Contributor Author

Eric-Arellano commented Aug 19, 2022

I was thinking the algorithm was auto-map if the abs path lies within the build root.

In that case, with find-links, they could use %(build_root)s interpolation, right? Which I should document here! That's a good trick. Edit: oh, duh, that won't transfer over to the lockfile persisted on disk.

With this auto-mapping, are you envisioning we would still allow you to configure --path-mappings manually too for non-build root locations? I could imagine wanting to share the same find links with multiple repositories.

Edit: maybe a middle-ground is to teach the recipe of using paths relative to the build root. This could be checked in to pants.toml for everyone, I believe:

[python-repos]
find_links = ["%(build_root)s/custom_wheels"]
path_mappings = ["BUILD_ROOT|%(build_root)s/custom_wheels"]

That requires no new code, and still gives flexibility for non-build-root path mappings.

@jsirois
Copy link
Contributor

jsirois commented Aug 19, 2022

I'm just envisioning the simplest thing, for any not already user custom path mapped path that lies in the repo, path map it for them, since it makes no sense in any situation I can think of to have a path laying in the repo be saved as it's idiosyncratic abs path on the machine the lockfile happened to be generated on.

@jsirois
Copy link
Contributor

jsirois commented Aug 19, 2022

I'm fine with incremental here if you don't have time as you say. We definitely need the manual option as an escape hatch at the least.

@Eric-Arellano
Copy link
Contributor Author

I'm fine with incremental here if you don't have time as you say. We definitely need the manual option as an escape hatch at the least.

I'm thinking manual option & document well the tip to leverage %(buildroot)s. That's great because then only the codebase admin has a one-time setup cost -- it avoids everyday users having to set up .pants.rc. This tip should work with other interpolation like $USER and $HOME too.

@jsirois
Copy link
Contributor

jsirois commented Aug 19, 2022

Sounds good. We'll let time tell us if users are confused or figure it out by reading docs.

@Eric-Arellano Eric-Arellano changed the title WIP: Support Pex's --path-mapping with lockfiles for better local requirement support Support Pex's --path-mapping with lockfiles for better local requirement support Aug 23, 2022
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]
# 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]
@Eric-Arellano Eric-Arellano added this to the 2.14.x milestone Aug 23, 2022
@Eric-Arellano Eric-Arellano marked this pull request as ready for review August 23, 2022 17:06
@jsirois
Copy link
Contributor

jsirois commented Aug 23, 2022

Eagerly erroring if users haven't appropriately configured

I cannot think of any case where this makes sense. On the lock creation side, its perfectly valid to have absolute paths for either find links repos or local projects. You can definitely set up your repo / dev machines / CI such that those paths are always the same; in which case everything works. On the lock consumption side, you could replicate Pex's existing logic to fail if there are path mappings in the lock but not on the CLI, but that seems to me - generically - like a road Pants should not go down. Pants failing fast for its own metadata is one thing. Failing fast on behalf of tools is another and seems almost always unwise. That would introduce brittleness for no trumping purpose I can see.

become `file://${{WHEELS_DIR}}/django-3.1.1-py3-none-any.whl`, where each user can
configure what WHEELS_DIR points to on their machine.

Expects values in the form `NAME|PATH`. You can specify multiple entries in the list.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is leaving off mention of the optional trailing |DESCRIPTION intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured to keep it simpler by not mentioning. The benefit of |DESCRIPTION is better error messages when --path-mappings is not defined at consumption side?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with keeping it simpler. If you have 4 find links repos, presumably it would be helpful if reading lock file in addition to error messages. Maybe not a worthwhile feature to have added though.

@Eric-Arellano
Copy link
Contributor Author

On the lock consumption side, you could replicate Pex's existing logic to fail if there are path mappings in the lock but not on the CLI, but that seems to me - generically - like a road Pants should not go down. Pants failing fast for its own metadata is one thing. Failing fast on behalf of tools is another and seems almost always unwise. That would introduce brittleness for no trumping purpose I can see.

Indeed, I had the same thought process and failed to write it down. This is specifically what I was referring to. And now that we have the interpolation tip, I think it's even less compelling because it's less common users will need to manually set up this option in .pants.rc.

# 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]
@Eric-Arellano Eric-Arellano merged commit 2f2fcb5 into pantsbuild:main Aug 24, 2022
@Eric-Arellano Eric-Arellano deleted the lock-path-mappings branch August 24, 2022 14:52
Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request Aug 24, 2022
…ement support (pantsbuild#16584)

Closes pantsbuild#16416.

As explained in the new docs, some users will be able to use fixed paths, which is the simplest approach. But others may have different paths per machine, e.g. using `/User/<username>` in the path. `--path-mappings` allows lockfile generators & consumers to set this on a per-machine basis.

## Tip to set to a common value

If you can use a common location and Pants's interpolation, e.g. `%(buildroot)s/wheels_dir`, then the config only needs to be set once. So, we encourage this approach.

This is deemed adequate compared to some other enhancements we could make:

- Trying to auto-detect when we can derive a common value.
- Eagerly erroring if users haven't appropriately configured `[python-repos].path_mapping`.

## Global option vs. per-resolve

The design doc at https://docs.google.com/document/d/1HAvpSNvNAHreFfvTAXavZGka-A3WWvPuH0sMjGUCo48/edit proposed having all resolve-related config be configurable on a per-resolve basis, rather than globally. We implemented this idea for a few options, but decided to not implement it (yet?) for `[python-repos].{find_links,indexes}` pantsbuild#16530.

Given that it is only possible to have a global setting for `[python-repos].find_links`, I could not come up with a compelling reason to let you have path mappings be per-resolve. It only complicated the user interface and `help` message because users have to think about resolves.

If we do end up implementing pantsbuild#16530, then it would make sense to also have this option be per-resolve. Until then, there is little value.
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]
Eric-Arellano added a commit that referenced this pull request Aug 29, 2022
…ement support (Cherry-pick of #16584) (#16625)

Closes #16416.

As explained in the new docs, some users will be able to use fixed paths, which is the simplest approach. But others may have different paths per machine, e.g. using `/User/<username>` in the path. `--path-mappings` allows lockfile generators & consumers to set this on a per-machine basis.

## Tip to set to a common value

If you can use a common location and Pants's interpolation, e.g. `%(buildroot)s/wheels_dir`, then the config only needs to be set once. So, we encourage this approach.

This is deemed adequate compared to some other enhancements we could make:

- Trying to auto-detect when we can derive a common value.
- Eagerly erroring if users haven't appropriately configured `[python-repos].path_mapping`.

## Global option vs. per-resolve

The design doc at https://docs.google.com/document/d/1HAvpSNvNAHreFfvTAXavZGka-A3WWvPuH0sMjGUCo48/edit proposed having all resolve-related config be configurable on a per-resolve basis, rather than globally. We implemented this idea for a few options, but decided to not implement it (yet?) for `[python-repos].{find_links,indexes}` #16530.

Given that it is only possible to have a global setting for `[python-repos].find_links`, I could not come up with a compelling reason to let you have path mappings be per-resolve. It only complicated the user interface and `help` message because users have to think about resolves.

If we do end up implementing #16530, then it would make sense to also have this option be per-resolve. Until then, there is little value.

[ci skip-rust]
cczona pushed a commit to cczona/pants that referenced this pull request Sep 1, 2022
…ement support (pantsbuild#16584)

Closes pantsbuild#16416.

As explained in the new docs, some users will be able to use fixed paths, which is the simplest approach. But others may have different paths per machine, e.g. using `/User/<username>` in the path. `--path-mappings` allows lockfile generators & consumers to set this on a per-machine basis.

## Tip to set to a common value

If you can use a common location and Pants's interpolation, e.g. `%(buildroot)s/wheels_dir`, then the config only needs to be set once. So, we encourage this approach.

This is deemed adequate compared to some other enhancements we could make:

- Trying to auto-detect when we can derive a common value.
- Eagerly erroring if users haven't appropriately configured `[python-repos].path_mapping`.

## Global option vs. per-resolve

The design doc at https://docs.google.com/document/d/1HAvpSNvNAHreFfvTAXavZGka-A3WWvPuH0sMjGUCo48/edit proposed having all resolve-related config be configurable on a per-resolve basis, rather than globally. We implemented this idea for a few options, but decided to not implement it (yet?) for `[python-repos].{find_links,indexes}` pantsbuild#16530.

Given that it is only possible to have a global setting for `[python-repos].find_links`, I could not come up with a compelling reason to let you have path mappings be per-resolve. It only complicated the user interface and `help` message because users have to think about resolves.

If we do end up implementing pantsbuild#16530, then it would make sense to also have this option be per-resolve. Until then, there is little value.
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.

Wire up Pex's --path-mapping to Pants for local requirement support with lockfiles
2 participants