feat(gazelle): pure golang helper #1895

hunshcn · 2024-05-13T13:58:57Z

Remove gazelle plugin's python deps and make it hermetic. No more relying on the system interpreter.

Use TreeSitter to parse Python code and use https://github.com/pypi/stdlib-list to determine whether a module is in std lib.

Fixes #1825
Fixes #1599
Related #1315

aignas

This change is very exciting, so thank you very much for working on this.

Could you change the PR description to remove the words "pain in the ass", please? This will become a commit message and I would love to keep the git commit messages more formal.

aignas · 2024-05-14T01:54:42Z

gazelle/def.bzl

+        downloaded_file_path = "3.11.txt",
+    )
+
+non_module_deps = module_extension(implementation = non_module_deps_impl)


If you use bazel-skylib 1.6.1 you could use the same as was done in #1892.

aignas · 2024-05-14T14:09:14Z

gazelle/python/BUILD.bazel

    #
    # See following for more info:
    # https://github.com/bazelbuild/bazel-gazelle/issues/1513
-    embedsrcs = [":helper.zip"],  # keep
+    embedsrcs = ["stdlib_list.txt"],  # keep


Should we include more lists and switch based on a value in the gazelle manifest?

This is not a simple matter. Because there may be multiple python version in monorepo, this may require a new directive. I suggest keeping the status quo in this pr and using stdlist of 3.11.

SGTM, could you please create an issue in the backlog to add this feature? I agree that having a directive or solving this issue in some way would be useful.

gazelle/python/BUILD.bazel

gazelle/python/file_parser.go

CHANGELOG.md

gazelle/def.bzl

aignas

LGTM in general, a few minor additions.

gazelle/WORKSPACE

gazelle/python/BUILD.bazel

gazelle/python/file_parser.go

gazelle/python/file_parser_test.go

aignas · 2024-05-15T09:10:16Z

@dougthor42, @linzhp, could you please test this PR in your repos? Does this speed things up? Does it slow things down? Since you two have bigger repos, it would be nice to use them as tests to ensure there are no hidden edge cases.

linzhp · 2024-05-15T14:45:34Z

Very excited to see this! I will test it in Uber later this week if that's ok.

dougthor42 · 2024-05-15T23:47:10Z

Oh this looks really great! Let's see how it does...

/cc @ssmall, another Googler who will be using this.

Summary

This branch causes Gazelle to take ~35% of the time, a savings of 40s for me. Hot damn that's awesome!

"Before this PR" commit: 55f31a3
"After this PR" commit: f6a190a

Before this PR	After this PR	Delta	%
60.8 s	21.2 s	40.6 s	~35%

Background info:

4731 python files. find src -type f -name "*.py" | wc -l
1618 directories. find src -type d -not -name "__pycache__" | wc -l
499 BUILD.bazel files previously generated by Gazelle. find src -type f -name "BUILD.bazel" | wc -l
~220 defined Python requirements in requirements.in
bazel version: 7.1.1
24 core AMD EPYC vCPU
96 GB RAM
SSD
Using experimental bazel downloader for pip.parse? yes.

$ lsb_release -a
No LSB modules are available.
Distributor ID: Debian
Description:    Debian GNU/Linux rodete
Release:        n/a
Codename:       rodete

Test:

Run hyperfine --warmup 1 --runs 5 'bazel run //:gazelle'
- only 5 runs instead of the default 10 because the old method takes >1min and I'm impatient.
- warmup allows bazel downloader to cache my project's python dependencies
Make sure that the git status of the test project is clean.

Before this PR:

Commit 55f31a3

$ hyperfine --warmup 1 --runs 5 'bazel run //:gazelle'
Benchmark 1: bazel run //:gazelle
  Time (mean ± σ):     60.756 s ±  0.867 s    [User: 53.651 s, System: 7.891 s]
  Range (min … max):   60.018 s … 61.873 s    5 runs

After this PR:

Commit f6a190a

$ hyperfine --warmup 1 --runs 5 'bazel run //:gazelle'                                                                                                                                                                                                                                                                        
Benchmark 1: bazel run //:gazelle
  Time (mean ± σ):     21.190 s ±  0.174 s    [User: 27.083 s, System: 2.679 s]
  Range (min … max):   20.964 s … 21.392 s    5 runs

gazelle/python/BUILD.bazel

aignas · 2024-05-17T02:16:11Z

gazelle/python/python_test.go

@@ -42,9 +41,10 @@ const (
 	gazelleBinaryName = "gazelle_binary"
 )

-var gazellePath = mustFindGazelle()
+var gazellePath string


Why is this necessary? Since this is a global variable, it may be better to set it in TestMain or as it was done previously?

var gazellePath = mustFindGazelle() will make go test . failed. Although this will not affect bazel test.

I simply modified it, and no longer use global variables. There is no need for global variables here.

aignas · 2024-05-17T02:19:37Z

gazelle/def.bzl

 GAZELLE_PYTHON_RUNTIME_DEPS = [
 ]
+
+non_module_deps = modules.as_extension(


Could you please move this into gazelle/python/private:extensions.bzl? please? The modules.as_extension is something that is not our external API within the gazelle plugin, so it should not be a part of def.bzl.

linzhp · 2024-05-19T21:11:14Z

This works in Uber

aignas

LGTM, thank you so much for this contribution!

This reverts commit 7fc7962. Fixes bazelbuild#1913

hunshcn force-pushed the feat/go-helper branch 5 times, most recently from edbbe93 to 15f80a4 Compare May 14, 2024 06:56

hunshcn marked this pull request as ready for review May 14, 2024 07:34

hunshcn requested review from f0rmiga and rickeylev as code owners May 14, 2024 07:34

hunshcn force-pushed the feat/go-helper branch 2 times, most recently from 08b3a7a to b7080bc Compare May 14, 2024 07:41

aignas reviewed May 15, 2024

View reviewed changes

hunshcn force-pushed the feat/go-helper branch from 4846ddf to 2ad0971 Compare May 15, 2024 06:44

hunshcn changed the title ~~feat(gazelle)!: pure golang helper~~ feat(gazelle): pure golang helper May 15, 2024

aignas reviewed May 15, 2024

View reviewed changes

gazelle/WORKSPACE Show resolved Hide resolved

gazelle/python/BUILD.bazel Outdated Show resolved Hide resolved

gazelle/python/file_parser.go Show resolved Hide resolved

gazelle/python/file_parser_test.go Outdated Show resolved Hide resolved

hunshcn mentioned this pull request May 15, 2024

[FR] (gazelle) Use specified python version to determine whether a module is a stdlib #1901

Open

aignas reviewed May 16, 2024

View reviewed changes

gazelle/python/BUILD.bazel Outdated Show resolved Hide resolved

hunshcn force-pushed the feat/go-helper branch 3 times, most recently from 0272c96 to 7a107e7 Compare May 16, 2024 14:26

hunshcn added 8 commits May 16, 2024 23:00

feat(gazelle)!: pure golang helper

344e0ae

use go_default_library

4698cb5

use errgroup

a64caf3

more tests

ec2c5a2

update about review

2f28fac

add licenses

431478b

fix test

bcfc95f

nit

bcd7f89

hunshcn added 3 commits May 16, 2024 23:00

use symlink for stdlib_list.txt

ad64cbc

stdlib_list of multi python versions

b57de8a

fix buildifier lint

86faa07

hunshcn force-pushed the feat/go-helper branch from 7a107e7 to 86faa07 Compare May 16, 2024 15:00

aignas reviewed May 17, 2024

View reviewed changes

hunshcn force-pushed the feat/go-helper branch 2 times, most recently from e0cf1d6 to 78aec8f Compare May 17, 2024 03:57

optimize code style, according to review

a46b206

hunshcn force-pushed the feat/go-helper branch from 78aec8f to a46b206 Compare May 17, 2024 03:59

aignas approved these changes May 20, 2024

View reviewed changes

aignas added this pull request to the merge queue May 20, 2024

Merged via the queue into bazelbuild:main with commit 7fc7962 May 20, 2024
4 checks passed

linzhp mentioned this pull request May 22, 2024

Cannot cross-compile Gazelle extension #1913

Open

aignas added a commit to aignas/rules_python that referenced this pull request May 30, 2024

Revert "feat(gazelle): pure golang helper (bazelbuild#1895)"

3bd96a9

This reverts commit 7fc7962. Fixes bazelbuild#1913

aignas mentioned this pull request May 30, 2024

revert(gazelle): revert #1895 as it is causing issues cross-building the plugin #1931

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gazelle): pure golang helper #1895

feat(gazelle): pure golang helper #1895

hunshcn commented May 13, 2024 •

edited by aignas

aignas left a comment

aignas May 14, 2024

hunshcn May 15, 2024

aignas May 14, 2024

hunshcn May 15, 2024

aignas May 15, 2024

aignas left a comment

aignas commented May 15, 2024 •

edited

linzhp commented May 15, 2024 •

edited

dougthor42 commented May 15, 2024

aignas May 17, 2024

hunshcn May 17, 2024

aignas May 17, 2024

linzhp commented May 19, 2024

aignas left a comment

feat(gazelle): pure golang helper #1895

feat(gazelle): pure golang helper #1895

Conversation

hunshcn commented May 13, 2024 • edited by aignas

aignas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aignas left a comment

Choose a reason for hiding this comment

aignas commented May 15, 2024 • edited

linzhp commented May 15, 2024 • edited

dougthor42 commented May 15, 2024

Summary

Background info:

Test:

Before this PR:

After this PR:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linzhp commented May 19, 2024

aignas left a comment

Choose a reason for hiding this comment

hunshcn commented May 13, 2024 •

edited by aignas

aignas commented May 15, 2024 •

edited

linzhp commented May 15, 2024 •

edited