Local relative requirements compile fails under 0.10.0+

[aaliddell]

🐞 bug report

Affected Rule

compile_pip_requirements

Is this a regression?

Yes. Works in 0.9.0, broken in 0.10.0+

Description

Setup is as here: #366

Under 0.10.0 and above, relative requirements fail due to not being able to resolve the file path.

Bug was introduced in aef17ad

Issue is the removal of os.chdir:

rules_python/python/pip_install/pip_compile.py

Lines 44 to 52 in dae610b

	elif "BUILD_WORKSPACE_DIRECTORY" in os.environ:
	# This value, populated when running under `bazel run`, is a path to the
	# "root of the workspace where the build was run."
	# This matches up with the values passed in via the macro using the 'rootpath' Make variable,
	# which for source files provides a path "relative to your workspace root."
	#
	# Changing to the WORKSPACE root avoids 'file not found' errors when the `.update` target is run
	# from different directories within the WORKSPACE.
	os.chdir(os.environ["BUILD_WORKSPACE_DIRECTORY"])

Became:

rules_python/python/pip_install/pip_compile.py

Lines 49 to 57 in aef17ad

	elif "BUILD_WORKSPACE_DIRECTORY" in os.environ:
	# This value, populated when running under `bazel run`, is a path to the
	# "root of the workspace where the build was run."
	# This matches up with the values passed in via the macro using the 'rootpath' Make variable,
	# which for source files provides a path "relative to your workspace root."
	#
	# Changing to the WORKSPACE root avoids 'file not found' errors when the `.update` target is run
	# from different directories within the WORKSPACE.
	requirements_txt = os.path.join(os.environ["BUILD_WORKSPACE_DIRECTORY"], requirements_txt)

🔬 Minimal Reproduction

See #366

🔥 Exception or Error

WARNING:pip._internal.req.constructors:Requirement 'deps/some_name/some_name-1.2.3-py3-none-any.whl' looks like a filename, but the file does not exist

🌍 Your Environment

Operating System:

  
Ubuntu 20.04.3 LTS
  

Output of bazel version:

  
bazel 5.0.0
  

Rules_python version:

  
0.10.2
  

Anything else relevant?

[groodt]

In general, we're considering removing support for relative requirements and reserving the use of third_party dependencies to be retrieved from PyPI or another mirror. Can you explain your use-case a bit better so we understand? In general, if you've got the wheels vendored into the repo, there are other rules you can use (namely whl_library) which would be preferable to us.

[aaliddell]

#366 documents whl_library being tried but it is a poor alternative, as it obviously does not resolve and fetch the dependencies of the local wheel. This means dependencies of the wheels must be manually copied into requirements.txt (and tracked when they get updated). Also, the relative requirements work for all dep types (tar.gz etc), not just for wheels.

The motivation for using this is the need for private package files that cannot be uploaded to PyPI. In an ideal world a private pip mirror would be used, but setting one up for just 1 or 2 packages is a significant maintenance burden, especially since there is still no good authentication mechanism with pip private mirrors (GitHub is still struggling to deliver private Python registries for potentially this reason). This leads to the 'easy' path being just vendoring the few KB of wheel files within the repo.

Since we have support currently and this is something permitted by pip, what's the motivation for removing it?

[groodt]

Thanks for the context. Will need to think about it and determine what is broken and why and whether it could be supported longer term. Out of interest, are you a user of pip_install or pip_parse?

no good authentication mechanism with pip private mirrors

I'm surprised by this. I thought pip supported .netrc files. I'll do some more research as I admit I've not ever needed to use it.

what's the motivation for removing it?

To be clear, no decision has been made yet and the functionality wasn't broken intentionally. However, python distribution packaging is very complex and a desire to simplify it to "remote only" might make vendoring dependency closures into bazel http_file easier, but perhaps there's ways to support both. :)

permitted by pip

These rules currently use pip and abuse requirements.txt slightly as a lockfile, but there should ultimately be the flexibility to move away from pip if the maintainers choose. In that sense, the name of the rules pip are a bit of a historical misnomer, in that they probably should either be pypi_ or some other package_resolver if we ever end up building a custom resolver (like pants ended up doing). Now, this is unlikely to happen anytime soon and may never happen, but only wanting to clarify that these rules need not support everything pip supports under all circumstances.

[aaliddell]

Out of interest, are you a user of pip_install or pip_parse?

Still on pip_install, as I understand pip_parse does not support local references at all?

I'm surprised by this. I thought pip supported .netrc files. I'll do some more research as I admit I've not ever needed to use it.

It's been a while since we investigated, but at the time we elected to use vendored deps it was becoming a significant project just to host a few wheels. It'd be great if we could just put them in a private GCS/S3 bucket and have pip know how to auth against that, but that doesn't seem possible yet. See here for an example of the infrastructure required just to host a private repo without resorting to just using 'unguessable public URLs'.

Interestingly the PSF would accept funding to fix this, so I'm surprised a sufficiently large interested company like GitHub (who appear to want to host private Python registries) haven't looked into this.

To be clear, no decision has been made yet and the functionality wasn't broken intentionally

What is your opinion on a PR to revert the removal of os.chdir in the short term? I haven't kept track enough of how this will break other things, so don't want to blindly make a PR that's going to do more harm than good. For now I can stay at 0.9.0, but I expect there'll be useful changes in future versions that we'll want.

These rules currently use pip and abuse requirements.txt slightly as a lockfile, but there should ultimately be the flexibility to move away from pip if the maintainers choose

Agreed, although the requirements spec is mostly independent of pip, so support for local references is documented as something any resolver could support, even if implementations may choose to not implement that (as I believe pip did until relatively recently, 2019?). If it gets to the point where a custom resolver is being considered, I can hopefully help you out with test cases or examples if desired.

[alexeagle]

I think we should just revert aef17ad that you've identified as the regression here. As far as we know, the problem it solved, #689 was only discovered by one user and not affecting others.

[groodt]

@aaliddell This should now be closed after reverting what we believe is the offending change. Please just be aware that this "relative behaviour" to a file in the repository may become difficult to continue supporting.

[aaliddell]

Ok, thanks 👍 I’ll checkout the new release tomorrow.

Is the relative behaviour impossible to support with the newer rules layout in pip_parse or just something that needs time some put in? I ask because if it’s the latter, I can (try) put together a PR for this. This is assuming this is a change you’d like?

It’s hard to gauge how much interest there really is for this, since I’m aware that supporting features for a tiny fraction of users is a pain; the original PR had a few others who were keen at the time, but it’s been a while since…

[jdimatteo]

This looks like it is broken again as of 0.17.3 :(