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

feat(gazelle): Remove integrity field from Gazelle manifest #1666

Merged
merged 1 commit into from
Feb 8, 2024

Conversation

adzenith
Copy link
Contributor

@adzenith adzenith commented Jan 4, 2024

As per the discussion in #1465 and in this PR, this PR does the following:

  • Make the requirements parameter to the gazelle_python_manifest macro optional.
  • If requirements is not provided, no integrity field is added to the manifest, and diff_test is used to see if the manifest is stale instead of the existing go_test that just checks the integrity.
  • Migrates go_binary to genrule to generate the manifest file that can be used with diff_test. (There's still a go_binary under the hood, but the manifest macro itself uses a genrule now rather than a wrapper go_binary.)
  • It does not migrate the manifest generator from Go to Python; hopefully that can be deferred to another PR.

A custom copy_to_source.sh script is included, which is used to update the manifest in the source tree.

Compatibility discussion:

  • The update and test target names have not changed; they are still //:gazelle_python_manifest.update and //:gazelle_python_manifest.test.

@f0rmiga
Copy link
Collaborator

f0rmiga commented Jan 4, 2024

When I introduced this integrity verification, the intent was to have speedy checks on whether the manifest was up to date without re-generating it. Did you consider this?

@adzenith
Copy link
Contributor Author

adzenith commented Jan 4, 2024

@f0rmiga in my experience, and the reason I opened #1465, in a large repo the integrity field is an extremely common source of merge conflicts. The test itself pretty much only runs in our CI system; locally we always run update anyway so we don't notice any speed gains from a faster test. (And if you've run update before, then the test is fast anyway because everything gets cached.)

All in all, in our case it's a huge benefit to no longer run into these merge conflicts, because in the end they take much more engineering time to deal with than running the test. What do you think?

@aignas
Copy link
Collaborator

aignas commented Jan 4, 2024

@adzenith, what is the time it takes to perform the check for staleness in the following scenarios:

  • no pypi deps present in repositoly cache.
  • pypi deps are present in the repository cache.

If we are removing this in the sake of simplicity it would be great to know the trade offs.

In reality I found that when having multiple requirements files (one for each os) this extra field may add extra mental overhead to understand how everything works, especially if the requirements files are updated incrementally.

@adzenith
Copy link
Contributor Author

adzenith commented Jan 4, 2024

I just tested on our monorepo. With the integrity field, both with and without pypi repository cache it takes ~0.7s. Without the integrity field, if we have our pip deps cached it takes the same amount of time at about 0.7s.

In our case we depend on pytorch, which is 2.3GB. So a "cold" check takes about 3 minutes in our case - but in actuality we never encounter this case, because pytorch (and most of the pip deps) are downloaded and cached during the course of normal work. In real use cases it would probably be just a few seconds to download any pip deps that the developer doesn't have yet, and this would only need to happen once.

Copy link
Collaborator

@aignas aignas left a comment

Choose a reason for hiding this comment

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

I had a few questions, but at the first glance looks like the right direction.

gazelle/MODULE.bazel Outdated Show resolved Hide resolved
gazelle/manifest/generate/generate.go Outdated Show resolved Hide resolved
@adzenith
Copy link
Contributor Author

adzenith commented Jan 5, 2024

I changed the permissions to 0644. Let me know your thoughts on the aspect dependency!

@adzenith adzenith force-pushed the manifest-integrity-removal branch 2 times, most recently from 1aafcde to 2d869eb Compare January 5, 2024 18:35
@adzenith
Copy link
Contributor Author

adzenith commented Jan 5, 2024

Everything is passing CI except this one Windows job:

(18:36:11) ERROR: C:/b/bk-windows-rksv/bazel/rules-python-python/examples/build_file_generation/BUILD.bazel:12:25: Middleman _middlemen/requirements.update.exe-runfiles failed: error reading file '@@python39_x86_64-pc-windows-msvc//:Lib/distutils/__pycache__/util.cpython-39.pyc': C:\b\igshgcm6\external\python39_x86_64-pc-windows-msvc\Lib\distutils\__pycache__\util.cpython-39.pyc (The process cannot access the file because it is being used by another process)

not sure what's going on there... have you seen this before?

@aignas
Copy link
Collaborator

aignas commented Jan 5, 2024

Everything is passing CI except this one Windows job:

(18:36:11) ERROR: C:/b/bk-windows-rksv/bazel/rules-python-python/examples/build_file_generation/BUILD.bazel:12:25: Middleman _middlemen/requirements.update.exe-runfiles failed: error reading file '@@python39_x86_64-pc-windows-msvc//:Lib/distutils/__pycache__/util.cpython-39.pyc': C:\b\igshgcm6\external\python39_x86_64-pc-windows-msvc\Lib\distutils\__pycache__\util.cpython-39.pyc (The process cannot access the file because it is being used by another process)

not sure what's going on there... have you seen this before?

This is a Windows CI flake

@f0rmiga
Copy link
Collaborator

f0rmiga commented Jan 7, 2024

@f0rmiga in my experience, and the reason I opened #1465, in a large repo the integrity field is an extremely common source of merge conflicts. The test itself pretty much only runs in our CI system; locally we always run update anyway so we don't notice any speed gains from a faster test. (And if you've run update before, then the test is fast anyway because everything gets cached.)

All in all, in our case it's a huge benefit to no longer run into these merge conflicts, because in the end they take much more engineering time to deal with than running the test. What do you think?

You are missing the case when this test is performed on CI without a separate pypi cache, which is the most common case.

@adzenith
Copy link
Contributor Author

adzenith commented Jan 8, 2024

I would assume that CI would either maintain the Bazel cache between runs (this is what we do), or be running py_test targets that already depend on these libraries anyway, or both. Would there be a case where this test would be run to check the Gazelle manifest but pip dependencies wouldn't otherwise be needed in the CI run?

@aignas
Copy link
Collaborator

aignas commented Jan 9, 2024

I guess the thing is that bazel test ... in the usual case will download all of the used pip dependencies in case the analysis cache was discarded in the previous run, so we need to download the third-party deps anyway. The only case where I see the lightweight gazelle manifest would help is to support a use-case of a quick lint type CI run, which can run really fast and not need to download all of the PyPI dependencies just for that. However, as @adzenith noted, I would expect it to be run on a runner which has all of the dependencies anyway.

@f0rmiga, are we missing something else here? If I remember you were working at Aspect at the time of authoring this and I am wondering if this work was sponsored by some client who had some specific requirements?

@f0rmiga
Copy link
Collaborator

f0rmiga commented Jan 9, 2024

We have to be careful every time we use assumptions to make changes to a repository that countless repositories depend on. We can't force everyone to do what we believe to be the best workflow.

@mattem can you shed some light here? You have a more recent experience with a very large Python repository.

Copy link
Collaborator

@f0rmiga f0rmiga left a comment

Choose a reason for hiding this comment

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

Blocking merge on this while we discuss further.

@mattem
Copy link
Collaborator

mattem commented Jan 9, 2024

I don't think we've ever hit conflicts on the integrity field, but I think that depends on if the repo has one requirements or many (we have 1000's), so the likelihood of conflicts is reduced (fwiw, the main contention I see is from the via comments from pip compile).

I'm on the fence about the removal of the field, however if it is removed, I don't think I agree with adding Aspect's bazel-lib as a dependency (seems odd for a core ruleset to depend on a 3p library).

I'm also a -1 on "manifest generator from Go to Python", just for the speed of running the tool I'd prefer to keep it as a native binary.

@lpulley
Copy link
Contributor

lpulley commented Jan 9, 2024

I don't think we've ever hit conflicts on the integrity field, but I think that depends on if the repo has one requirements or many (we have 1000's), so the likelihood of conflicts is reduced (fwiw, the main contention I see is from the via comments from pip compile).

We have a monorepo with a 3k-line pip lockfile. When we submitted the Gazelle manifest last week, we had lots of conflicts -- so many that we nearly reverted the manifest. This PR was perfectly timed, so I grabbed the diff as a Bazel repository patch and we've been running with it for a few days now. It seems to be working well for us (anecdotally).

I don't have strong feelings about the manner by which it is removed (i.e. 3p libs etc.) but it definitely has to go for this manifest to be usable in a monorepo (with a mono-lockfile). That's my two cents.

@lpulley
Copy link
Contributor

lpulley commented Jan 9, 2024

Perhaps it could be configurable/opt-in/opt-out?

@aignas
Copy link
Collaborator

aignas commented Jan 9, 2024

I also wanted to propose this to be an opt out/in setting. We could make the requirements file passing an optional field, which would either use the go_test (old code) or the new code.

Experience from @lpulley mirrors my experience as well and we opted to disabling the integrity check by using a different file as the source for the integrity.

@f0rmiga, any other ideas how monorepos with many/single requirements lock files could coexist?

@f0rmiga
Copy link
Collaborator

f0rmiga commented Jan 13, 2024

I think an opt out feature is ideal since it doesn't require everyone else to opt in, and it still satisfies repos that change the requirements too often.

@aignas
Copy link
Collaborator

aignas commented Jan 15, 2024

@f0rmiga so I guess we are on the same page here that the requirements attribute for the gazelle manifest generation should be made optional and based on this we would be either including the integrity hash in the manifest or not. I think updating the examples within the rules_python repository to not have the integrity hash would help with dependabot dependency updates, which I think would facilitate the maintenance of the repo.

@adzenith, what do you think?

@adzenith
Copy link
Contributor Author

Works for me. I'll add an argument to the manifest target that lets you pick whether there's an integrity field. I'll default it to having the integrity there for back compat.

@f0rmiga
Copy link
Collaborator

f0rmiga commented Jan 17, 2024

Awesome. Thanks for the contribution, @adzenith!

@adzenith
Copy link
Contributor Author

Ok, updated with a new use_integrity_field parameter to the gazelle_python_manifest macro. Let me know what you think!

gazelle/manifest/copy_to_source.sh Outdated Show resolved Hide resolved
@@ -52,6 +52,7 @@ gazelle_python_manifest(
"//:requirements_windows.txt",
],
tags = ["exclusive"],
use_integrity_field = False,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the thinking behind having use_integrity_field and requirements passed into the macro? IIRC, requirements are only used to generate the integrity field. Do you think we get a better API that way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

requirements is now a kwarg and the value is only used if use_integrity_field is set. I think having use_integrity_field be explicit is nice, but I'm also happy to make it just skip the integrity if requirements is unset. Let me know your thoughts.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd be fine with using the requirements arg as a switch as I am not sure I see a usecase where having two args would be useful. @f0rmiga, let us know if you would like to have this toggled explicitly.

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've removed the use_integrity_field argument in favor of switching based on whether requirements is set.

@adzenith adzenith force-pushed the manifest-integrity-removal branch 13 times, most recently from 0bb086a to 9e08f1a Compare February 8, 2024 07:08
@adzenith adzenith force-pushed the manifest-integrity-removal branch 4 times, most recently from 02863ad to b4d1c53 Compare February 8, 2024 07:28
Copy link
Collaborator

@f0rmiga f0rmiga left a comment

Choose a reason for hiding this comment

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

Ship it, fast, before Windows decides to throw a tantrum!

@f0rmiga f0rmiga added this pull request to the merge queue Feb 8, 2024
Merged via the queue into bazelbuild:main with commit 677fb53 Feb 8, 2024
4 checks passed
@adzenith adzenith deleted the manifest-integrity-removal branch February 8, 2024 07:57
@adzenith
Copy link
Contributor Author

adzenith commented Feb 8, 2024

Thanks all for the reviews and the feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants