refactor: use click in dependency_resolver.py #1071

cj81499 · 2023-02-14T16:55:40Z

Using click makes it easier to parse arguments. Many args are now named arguments
(options), and the need for using positional args with stub "None" values isn't
necessary anymore.

There is already a dependency on click via piptools, so this doesn't introduce a new
dependency.

Relates to #1067

google-cla · 2023-02-14T16:55:44Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

cj81499 · 2023-06-26T20:03:56Z

@hrfuller Apologies for the delay. This is ready for review!

cj81499 · 2023-07-06T19:38:47Z

@hrfuller are you available to take a look at this? If not, is there someone else who might be able to?

hrfuller · 2023-07-06T20:33:42Z

are you available to take a look at this? If not, is there someone else who might be able to?

Yes, I can take a look. @rickeylev is a more active maintainer than me these days so I'd like him to take a look as well.

hrfuller

Overall looks like an improvement to me. Thanks!

hrfuller · 2023-07-06T20:45:39Z

python/pip_install/tools/dependency_resolver/dependency_resolver.py

@@ -128,6 +131,8 @@ def _locate(bazel_runfiles, file):
    os.environ["LC_ALL"] = "C.UTF-8"
    os.environ["LANG"] = "C.UTF-8"

+    argv = []


It looks like we lose sys.argv[0] compared to before. I'm not sure if the piptools cli expects argv to have argv[0] be the name of the original script but maybe we can add sys.argv[0] here in case.

Just to be clear, are you suggesting we change this to the following?

Suggested change

argv = []

argv = [sys.argv[0]]

From what I can tell, omitting argv[0] when calling cli() is correct.

Under the hood, piptools is using click, and the implementation is obscured by a ton of decorators. Digging into it...

compile.cli is an instance of the click Command class.

Command.__call__ is an alias for Command.main

Command.main docs say, for the first positionl arg (named args): "the arguments that should be used for parsing. If not provided sys.argv[1:] is used"

So it doesn't expect args to contain argv[0]. The docs go on further to say the optional prog_name arg is initialized to sys.argv[0] if not specified.

As a side note, there is a standalone_mode arg, which controls whether the command tries to exit or not.

hrfuller · 2023-07-06T20:48:51Z

python/pip_install/tools/dependency_resolver/dependency_resolver.py

        requirements_in_relative
        if Path(requirements_in_relative).exists()
        else resolved_requirements_in
    )
-    print(sys.argv)


Unclear why this was here. Probably for debugging?

That was my assumption too. Didn't seem like something that ought to stick around (adding clutter to stdout!).

Happy to add it back if someone has a good reason for it to stay there.

cj81499

@rickeylev is a more active maintainer than me these days so I'd like him to take a look as well.

@hrfuller I take this to mean you'd like to hold off on merging until they review?

cj81499 · 2023-07-11T15:48:12Z

python/pip_install/tools/dependency_resolver/dependency_resolver.py

@@ -128,6 +131,8 @@ def _locate(bazel_runfiles, file):
    os.environ["LC_ALL"] = "C.UTF-8"
    os.environ["LANG"] = "C.UTF-8"

+    argv = []


Just to be clear, are you suggesting we change this to the following?

Suggested change

argv = []

argv = [sys.argv[0]]

cj81499 · 2023-07-11T15:49:40Z

python/pip_install/tools/dependency_resolver/dependency_resolver.py

        requirements_in_relative
        if Path(requirements_in_relative).exists()
        else resolved_requirements_in
    )
-    print(sys.argv)


That was my assumption too. Didn't seem like something that ought to stick around (adding clutter to stdout!).

Happy to add it back if someone has a good reason for it to stay there.

cj81499 · 2023-07-24T19:29:11Z

@hrfuller @rickeylev bump!

I would really like to get this (and the related #1067) merged.

cj81499 · 2023-08-21T14:55:52Z

@hrfuller @rickeylev I'd still like to get this (and #1067) merged.
Please take a look at your earliest convenience.

cj81499 · 2023-09-07T15:07:45Z

@f0rmiga I can't seem to get the attention of @hrfuller or @rickeylev and I'm not sure what else to try (other than contacting another codeowner). Perhaps you can help?

avoid mutating `sys.argv` Co-authored-by: Logan Pulley <lpulley@ocient.com>

rickeylev · 2023-11-01T18:37:23Z

python/pip_install/tools/dependency_resolver/dependency_resolver.py

@@ -128,6 +131,8 @@ def _locate(bazel_runfiles, file):
    os.environ["LC_ALL"] = "C.UTF-8"
    os.environ["LANG"] = "C.UTF-8"

+    argv = []


From what I can tell, omitting argv[0] when calling cli() is correct.

Under the hood, piptools is using click, and the implementation is obscured by a ton of decorators. Digging into it...

compile.cli is an instance of the click Command class.

Command.__call__ is an alias for Command.main

Command.main docs say, for the first positionl arg (named args): "the arguments that should be used for parsing. If not provided sys.argv[1:] is used"

So it doesn't expect args to contain argv[0]. The docs go on further to say the optional prog_name arg is initialized to sys.argv[0] if not specified.

As a side note, there is a standalone_mode arg, which controls whether the command tries to exit or not.

rickeylev · 2023-11-01T18:44:48Z

Hi @cj81499,

Thanks for your PR. Sorry it took so long to be reviewed. I had let this languish because I thought it was introducing a new dependency on click, but we already have that dependency, so this isn't as bad as I thought.

cj81499 · 2023-11-01T18:52:03Z

@rickeylev thank you for getting around to this! ❤️

fwiw, I did say the following in the PR description ;)

There is already a dependency on click via piptools, so this doesn't introduce a new dependency.

Excited that #1067 will be unblocked once this makes its way through the queue!

cj81499 requested a review from hrfuller as a code owner February 14, 2023 16:55

cj81499 mentioned this pull request Feb 14, 2023

feat: update compile_pip_requirements to support multiple input files #1067

Merged

cj81499 force-pushed the use-click branch from f554c7b to 74fd521 Compare May 10, 2023 15:44

cj81499 marked this pull request as draft May 11, 2023 16:03

cj81499 force-pushed the use-click branch 2 times, most recently from c40fb0e to b30e47f Compare June 13, 2023 00:17

daveboutcher approved these changes Jun 20, 2023

View reviewed changes

cj81499 marked this pull request as ready for review June 22, 2023 14:35

chrislovecnm requested a review from rickeylev July 6, 2023 19:59

hrfuller approved these changes Jul 6, 2023

View reviewed changes

cj81499 commented Jul 11, 2023

View reviewed changes

cj81499 force-pushed the use-click branch 2 times, most recently from 653b91f to 3b413d7 Compare October 31, 2023 18:15

refactor dependency_resolver.py to use click

5977727

avoid mutating `sys.argv` Co-authored-by: Logan Pulley <lpulley@ocient.com>

cj81499 force-pushed the use-click branch from 3b413d7 to 5977727 Compare November 1, 2023 16:04

rickeylev changed the title ~~refactor dependency_resolver.py to use click~~ refactor: use click in dependency_resolver.py Nov 1, 2023

rickeylev approved these changes Nov 1, 2023

View reviewed changes

rickeylev added this pull request to the merge queue Nov 1, 2023

Merged via the queue into bazelbuild:main with commit 4e26bcd Nov 1, 2023
3 checks passed

cj81499 deleted the use-click branch November 1, 2023 21:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use click in dependency_resolver.py #1071

refactor: use click in dependency_resolver.py #1071

cj81499 commented Feb 14, 2023 •

edited by rickeylev

Loading

google-cla bot commented Feb 14, 2023

cj81499 commented Jun 26, 2023

cj81499 commented Jul 6, 2023

hrfuller commented Jul 6, 2023 •

edited

Loading

hrfuller left a comment

hrfuller Jul 6, 2023

cj81499 Jul 11, 2023

rickeylev Nov 1, 2023

hrfuller Jul 6, 2023 •

edited

Loading

cj81499 Jul 11, 2023

cj81499 left a comment •

edited

Loading

cj81499 Jul 11, 2023

cj81499 Jul 11, 2023

cj81499 commented Jul 24, 2023

cj81499 commented Aug 21, 2023

cj81499 commented Sep 7, 2023

rickeylev Nov 1, 2023

rickeylev commented Nov 1, 2023

cj81499 commented Nov 1, 2023

refactor: use click in dependency_resolver.py #1071

refactor: use click in dependency_resolver.py #1071

Conversation

cj81499 commented Feb 14, 2023 • edited by rickeylev Loading

google-cla bot commented Feb 14, 2023

cj81499 commented Jun 26, 2023

cj81499 commented Jul 6, 2023

hrfuller commented Jul 6, 2023 • edited Loading

hrfuller left a comment

Choose a reason for hiding this comment

hrfuller Jul 6, 2023

Choose a reason for hiding this comment

cj81499 Jul 11, 2023

Choose a reason for hiding this comment

rickeylev Nov 1, 2023

Choose a reason for hiding this comment

hrfuller Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

cj81499 Jul 11, 2023

Choose a reason for hiding this comment

cj81499 left a comment • edited Loading

Choose a reason for hiding this comment

cj81499 Jul 11, 2023

Choose a reason for hiding this comment

cj81499 Jul 11, 2023

Choose a reason for hiding this comment

cj81499 commented Jul 24, 2023

cj81499 commented Aug 21, 2023

cj81499 commented Sep 7, 2023

rickeylev Nov 1, 2023

Choose a reason for hiding this comment

rickeylev commented Nov 1, 2023

cj81499 commented Nov 1, 2023

cj81499 commented Feb 14, 2023 •

edited by rickeylev

Loading

hrfuller commented Jul 6, 2023 •

edited

Loading

hrfuller Jul 6, 2023 •

edited

Loading

cj81499 left a comment •

edited

Loading