pip_compile: remove external/workspace_name prefix from generated requirements.txt #690

BoleynSu · 2022-04-16T15:24:42Z

Fixes #689

PR Checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature (please, look at the "Scope of the project" section in the README.md file)
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
Other... Please describe:

What is the current behavior?

Issue Number: #689

What is the new behavior?

In the generated file via -r filename will be replaced with via -r -.

Does this PR introduce a breaking change?

Yes
No

The existing generated tests will fail and a regeneration is required.

Other information

BoleynSu · 2022-04-16T15:25:41Z

The test is not added yet and I am expecting to break some existing ones.
I will fix them and add a new one once the PR is SGTMed.

BoleynSu · 2022-04-20T15:09:26Z

Friendly ping.

alexeagle · 2022-04-22T13:13:50Z

I don't think this is really an improvement - these from lines are useful documentation to say where a requirement came from, and the user doesn't understand what stdin is in this case, it's hidden by Bazel.

Can we just fixup the external pathing before writing the file instead?

BoleynSu · 2022-04-23T02:14:45Z

Sure. That makes sense. Just that's more work to do and I want to make sure the upstream maintainer would think the issue is acceptable to be fixed (which is not always the case) in the first place before I spend more time on it. Sent from https://boleyn.su/phone

…

On Fri, Apr 22, 2022, 21:14 Alex Eagle ***@***.***> wrote: I don't think this is really an improvement - these from lines are useful documentation to say where a requirement came from, and the user doesn't understand what stdin is in this case, it's hidden by Bazel. Can we just fixup the external pathing before writing the file instead? — Reply to this email directly, view it on GitHub <#690 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXVSKBYKEZLSBJASDHHLQ3VGKQZVANCNFSM5TSQKVLQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

BoleynSu · 2022-04-23T08:45:57Z

As a byproduct, this also allows |compile_pip_requirements| to comsume a generated requirements.in.
@alexeagle PTAL. I can add some tests after you think the PR is OK.

…uirements.txt

f0rmiga · 2022-05-03T05:18:36Z

@alexeagle Seems valid to me with proper tests. Anything else you want @BoleynSu to change?

alexeagle · 2022-05-03T12:48:45Z

Yup, seems like a targeted fix. Even a simple test by checking in a requirements file that currently has an external/ segment would help.

BoleynSu · 2022-05-03T13:08:04Z

Cool. I will add some tests later. I have implemented something simliar to @unpinned_maven//:pin which depend on this fix. Maybe I can use that as a test here.

BoleynSu · 2022-05-04T19:33:07Z

Hi @alexeagle and @f0rmiga, I have added the tests PTAL. Also I implemented something simliar to @unpinned_maven//:pin in the test. Would you mind also check if that can be part of the contributed repository rules?

alexeagle · 2022-05-04T22:48:45Z

tests/pip_deps/WORKSPACE

+
+PIP_PACKAGES = {"sphinx": "4.5.0"}
+
+pip_deps(


this was the intent of the pip_parse_vendored example introduced in 12662b6

could you just update that example instead of introducing another e2e test? They are kinda slow so it would be better not to accumulate more.

Also this doc 12662b6#diff-b92f94a67df862cde9714ff224afa676e39fe02da4fd4c6993daf684fbbc7cbdR182-R186 should maybe be updated?

this was the intent of the pip_parse_vendored example introduced in 12662b6

The major issue with pip_parse_vendored and the approach used for golang is that the meta data is unknown to Bazel. With pip_deps, one can save all the metadata in a .bzl file so that the downstream can consume it, e.g., use it to resolve diamond dependency.

I did something similar with golang dependencies at https://github.com/BoleynSu-Org/monorepo/blob/main/configs/deps/go_deps.bzl.

could you just update that example instead of introducing another e2e test? They are kinda slow so it would be better not to accumulate more.

Sure, I think I can merge it into another e2e test.

Also this doc 12662b6#diff-b92f94a67df862cde9714ff224afa676e39fe02da4fd4c6993daf684fbbc7cbdR182-R186 should maybe be updated?

alexeagle · 2022-05-04T22:50:10Z

.bazelci/presubmit.yml

@@ -36,5 +36,6 @@ platforms:
      - "-//gazelle/..."
      # The dependencies needed for this test are not cross-platform: https://github.com/bazelbuild/rules_python/issues/260
      - "-//tests:pip_repository_entry_points_example"
+      - "-//tests:pip_deps_example"


why doesn't it work on Windows?

I am not sure. I guessed it was because bazel_integration_test did not work well on Windows, which is likely wrong.

The error is caused by os.chdir("external\\unpinned_pip") reporting no such directory. I do not have a Windows machine to debug it at the moment. Would you mind take a look if possible?

python/pip_install/pip_compile.py

alexeagle · 2022-05-04T22:56:29Z

tests/pip_deps/pip_deps.bzl

+        python_interpreter_target = None,
+        **kwargs):
+    _requirements_in(
+        name = "unpinned_" + name,


I think the term "unpinned" may be inaccurate here. I'm not sure how it compares with rules_jvm_external, but here, the existing locked requirements file is still an input to pip-compile, so any already-pinned transitive dependencies will remain pinned to the same version. Only versions which no longer satisfy the constraints in requirements.in will actually change.

If the behavior is different from rules_jvm_external, then maybe another name would be better?

I think the term "unpinned" may be inaccurate here. I'm not sure how it compares with rules_jvm_external, but here, the existing locked requirements file is still an input to pip-compile, so any already-pinned transitive dependencies will remain pinned to the same version. Only versions which no longer satisfy the constraints in requirements.in will actually change.

The input to _requirements_in is packages and output is a requirements.in file. None of them have anything to do with the locked requirements file.

If the behavior is different from rules_jvm_external, then maybe another name would be better?

There's a requirements_lock attribute here that gets passed to pip_compile. Am I missing something?

I see. Yes, the locked requirements file is also part of the input.

It seems that [1] we can pass --upgrade to achive this. It only read the locked file when there is no --upgrade. I am also wondering why --upgrade is not passed already? I think people are expecting this. We would better document it somewhere.

Also, in my usecase the requirements.txt is always generated so I did not ever notice it.

[1] https://github.com/jazzband/pip-tools/blob/master/piptools/scripts/compile.py#L352

tests/pip_deps/pip_deps.bzl

BoleynSu · 2022-05-05T23:09:01Z

Hi @alexeagle, I will remove pip_deps for now and add the test to existing an e2e test. I will propose another PR regarding pip_deps if you think it is worth the effort. If so, maybe we can first decide a better name for it.

BoleynSu · 2022-05-08T04:28:33Z

Hi @alexeagle, I implemented another pip_deps at https://github.com/BoleynSu-Org/monorepo/blob/e819723afa334f5220097291969cd7c7bb419716/configs/deps/pip_deps.bzl which addressed this issue you pointed out. Thanks!

Use site:
https://github.com/BoleynSu-Org/monorepo/blob/e819723afa334f5220097291969cd7c7bb419716/configs/build/workspace_2.bzl#L21
https://github.com/BoleynSu-Org/monorepo/blob/e819723afa334f5220097291969cd7c7bb419716/configs/build/workspace_3.bzl#L7

Please let me know if you think this can be upstreamed.

alexeagle

Thanks! sorry this got stalled for so long.

alexeagle · 2022-06-21T23:44:43Z

I'll get this in so that #689 can be fixed.

…uirements.txt (bazelbuild#690) * pip_compile: remove external/workspace_name prefix from generated requirements.txt * add some tests and a demo impl of pip_deps * update pinned requirement Co-authored-by: Alex Eagle <alex@aspect.dev>

…ated requirements.txt (#690)" This reverts commit aef17ad.

BoleynSu requested review from brandjon and lberki as code owners April 16, 2022 15:24

BoleynSu force-pushed the fix689 branch from 155a662 to c683d42 Compare April 16, 2022 15:29

BoleynSu marked this pull request as draft April 20, 2022 15:08

alexeagle self-assigned this Apr 22, 2022

BoleynSu force-pushed the fix689 branch 2 times, most recently from 7d86772 to 34fb449 Compare April 23, 2022 08:41

BoleynSu changed the title ~~pip_compile: use stdin instead of a filename~~ pip_compile: remove external/workspace_name prefix from generated requirements.txt Apr 23, 2022

pip_compile: remove external/workspace_name prefix from generated req…

f332a62

…uirements.txt

BoleynSu force-pushed the fix689 branch from 34fb449 to f332a62 Compare April 24, 2022 15:53

BoleynSu marked this pull request as ready for review April 24, 2022 16:55

BoleynSu requested a review from thundergolfer as a code owner May 4, 2022 19:29

BoleynSu force-pushed the fix689 branch 3 times, most recently from e1549e9 to fc806a9 Compare May 4, 2022 20:07

add some tests and a demo impl of pip_deps

4103b86

BoleynSu force-pushed the fix689 branch from fc806a9 to 4103b86 Compare May 4, 2022 20:12

Merge branch 'main' into fix689

4fed83f

alexeagle reviewed May 4, 2022

View reviewed changes

alexeagle added 2 commits June 21, 2022 16:31

Merge branch 'main' into fix689

67090d4

update pinned requirement

4a65a73

alexeagle approved these changes Jun 21, 2022

View reviewed changes

alexeagle merged commit aef17ad into bazelbuild:main Jun 21, 2022

alexeagle added a commit that referenced this pull request Aug 17, 2022

Revert "pip_compile: remove external/workspace_name prefix from gener…

a24a236

…ated requirements.txt (#690)" This reverts commit aef17ad.

alexeagle added a commit that referenced this pull request Aug 17, 2022

Revert "pip_compile: remove external/workspace_name prefix from gener…

2b874fc

…ated requirements.txt (#690)" This reverts commit aef17ad.

alexeagle mentioned this pull request Aug 17, 2022

Revert "pip_compile: remove external/workspace_name prefix from gener… #794

Merged

alexeagle added a commit that referenced this pull request Aug 17, 2022

Revert "pip_compile: remove external/workspace_name prefix from gener…

29a1607

…ated requirements.txt (#690)" This reverts commit aef17ad.

alexeagle added a commit that referenced this pull request Aug 17, 2022

Revert "pip_compile: remove external/workspace_name prefix from gener…

cf4ef3d

…ated requirements.txt (#690)" This reverts commit aef17ad.

alexeagle added a commit that referenced this pull request Aug 18, 2022

Revert "pip_compile: remove external/workspace_name prefix from gener…

248d7a2

…ated requirements.txt (#690)" This reverts commit aef17ad.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pip_compile: remove external/workspace_name prefix from generated requirements.txt #690

pip_compile: remove external/workspace_name prefix from generated requirements.txt #690

BoleynSu commented Apr 16, 2022 •

edited

Loading

BoleynSu commented Apr 16, 2022

BoleynSu commented Apr 20, 2022

alexeagle commented Apr 22, 2022

BoleynSu commented Apr 23, 2022 via email

BoleynSu commented Apr 23, 2022

f0rmiga commented May 3, 2022

alexeagle commented May 3, 2022

BoleynSu commented May 3, 2022

BoleynSu commented May 4, 2022

alexeagle May 4, 2022

BoleynSu May 5, 2022

alexeagle May 4, 2022

BoleynSu May 5, 2022

alexeagle May 4, 2022

BoleynSu May 5, 2022 •

edited

Loading

alexeagle May 5, 2022

BoleynSu May 5, 2022 •

edited

Loading

BoleynSu commented May 5, 2022 •

edited

Loading

BoleynSu commented May 8, 2022 •

edited

Loading

alexeagle left a comment

alexeagle commented Jun 21, 2022


		PIP_PACKAGES = {"sphinx": "4.5.0"}

		pip_deps(

pip_compile: remove external/workspace_name prefix from generated requirements.txt #690

pip_compile: remove external/workspace_name prefix from generated requirements.txt #690

Conversation

BoleynSu commented Apr 16, 2022 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

BoleynSu commented Apr 16, 2022

BoleynSu commented Apr 20, 2022

alexeagle commented Apr 22, 2022

BoleynSu commented Apr 23, 2022 via email

BoleynSu commented Apr 23, 2022

f0rmiga commented May 3, 2022

alexeagle commented May 3, 2022

BoleynSu commented May 3, 2022

BoleynSu commented May 4, 2022

alexeagle May 4, 2022

Choose a reason for hiding this comment

BoleynSu May 5, 2022

Choose a reason for hiding this comment

alexeagle May 4, 2022

Choose a reason for hiding this comment

BoleynSu May 5, 2022

Choose a reason for hiding this comment

alexeagle May 4, 2022

Choose a reason for hiding this comment

BoleynSu May 5, 2022 • edited Loading

Choose a reason for hiding this comment

alexeagle May 5, 2022

Choose a reason for hiding this comment

BoleynSu May 5, 2022 • edited Loading

Choose a reason for hiding this comment

BoleynSu commented May 5, 2022 • edited Loading

BoleynSu commented May 8, 2022 • edited Loading

alexeagle left a comment

Choose a reason for hiding this comment

alexeagle commented Jun 21, 2022

BoleynSu commented Apr 16, 2022 •

edited

Loading

BoleynSu May 5, 2022 •

edited

Loading

BoleynSu May 5, 2022 •

edited

Loading

BoleynSu commented May 5, 2022 •

edited

Loading

BoleynSu commented May 8, 2022 •

edited

Loading