Skip to content

Fix pickle related race condition (fixes #1164, fixes #1204)#1243

Merged
bwendling merged 1 commit intogoogle:mainfrom
hartwork:fix-pickle-related-race-condition
Oct 7, 2024
Merged

Fix pickle related race condition (fixes #1164, fixes #1204)#1243
bwendling merged 1 commit intogoogle:mainfrom
hartwork:fix-pickle-related-race-condition

Conversation

@hartwork
Copy link
Contributor

@hartwork hartwork commented Oct 3, 2024 •
edited
Loading

Fixes #1164
Fixes #1204

I have flaky pre-commit CI caused by Yapf in multiple repositories by now and it was about time to start doing something about it… I'm grateful for the great analysis by both @a-gardner1 and @whlook.

The pull request sits on top of #1242 for the moment purely for a chance at a green CI: I'm happy to rebase off of it after that one has been merged.

For a convenient reproducer of the race condition based on earlier work by @whlook please see #1204 (comment) .

If this could be reviewed and merged soon-ish, that would rock the house. Thanks in advance! 🙏

CC @bwendling @whlook @a-gardner1 @kamahen

@a-gardner1
Copy link

a-gardner1 commented Oct 3, 2024

Looks good to me. Thanks for picking it up! Do you think a unit test could be added to verify that the reproducer is fixed and does not regress in the future?

@hartwork
Copy link
Contributor Author

hartwork commented Oct 3, 2024

Looks good to me. Thanks for picking it up! Do you think a unit test could be added to verify that the reproducer is fixed and does not regress in the future?

@a-gardner1 that's an interesting idea. I'm happy to add a test based on that reproducer once it's the last thing considered blocking a merge.

@hartwork
ylib2to3/pgen2/grammar: Fix Grammar.dump for parallel use with Gramma…
51ba17e 
…r.load

Previously, bad timing could make another process run into reading a
half-written pickle cache file, and thus fail like this:

> Traceback (most recent call last):
>   File "[..]/bin/yapf", line 5, in <module>
>     from yapf import run_main
>   File "[..]/lib/python3.11/site-packages/yapf/__init__.py", line 41, in <module>
>     from yapf.yapflib import yapf_api
>   File "[..]/lib/python3.11/site-packages/yapf/yapflib/yapf_api.py", line 38, in <module>
>     from yapf.pyparser import pyparser
>   File "[..]/lib/python3.11/site-packages/yapf/pyparser/pyparser.py", line 44, in <module>
>     from yapf.yapflib import format_token
>   File "[..]/lib/python3.11/site-packages/yapf/yapflib/format_token.py", line 23, in <module>
>     from yapf.pytree import pytree_utils
>   File "[..]/lib/python3.11/site-packages/yapf/pytree/pytree_utils.py", line 30, in <module>
>     from yapf_third_party._ylib2to3 import pygram
>   File "[..]/lib/python3.11/site-packages/yapf_third_party/_ylib2to3/pygram.py", line 29, in <module>
>     python_grammar = driver.load_grammar(_GRAMMAR_FILE)
>                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>   File "[..]/lib/python3.11/site-packages/yapf_third_party/_ylib2to3/pgen2/driver.py", line 252, in load_grammar
>     g.load(gp)
>   File "[..]/lib/python3.11/site-packages/yapf_third_party/_ylib2to3/pgen2/grammar.py", line 95, in load
>     d = pickle.load(f)
>         ^^^^^^^^^^^^^^
> EOFError: Ran out of input
@hartwork hartwork force-pushed the fix-pickle-related-race-condition branch from b472494 to 51ba17e Compare October 3, 2024 22:35
@hartwork
Copy link
Contributor Author

hartwork commented Oct 3, 2024

I'm happy to rebase off of it after that one has been merged.

Done.

@hartwork
Copy link
Contributor Author

hartwork commented Oct 6, 2024

@bwendling any chance you could review the fix this coming weak? The bug is quite a bummer and I'm happy to make adjustments where needed. Thank you! 🙏

@bwendling bwendling merged commit 7e21823 into google:main Oct 7, 2024
@hartwork
Copy link
Contributor Author

hartwork commented Oct 7, 2024

@bwendling many thanks for the review and merge! Is there a chance for a new release v0.40.3 include this pull request to get the fixes to the users? Please let me know if you could use any help on that end 🙏

@hartwork hartwork mentioned this pull request Oct 12, 2024
Closed
XuehaiPan added a commit to XuehaiPan/triton that referenced this pull request Oct 26, 2024
@XuehaiPan
[CI] bump yapf version from be72557 to 7e21823
6a36eda 
Commit `7e21823` fixes race condition when `pre-commit` running `yapf` in parallel.

See also:

- google/yapf#1243
@XuehaiPan XuehaiPan mentioned this pull request Oct 26, 2024
Merged
7 tasks
ThomasRaoux pushed a commit to triton-lang/triton that referenced this pull request Oct 28, 2024
@XuehaiPan
[CI] bump yapf version from be72557 to 7e21823 (#4997)
3889f3f 
This PR bumps `yapf` version from `be72557` to `7e21823` in pre-commit
hook. Commit `7e21823` fixes race condition when `pre-commit` running
`yapf` in parallel.

Use a sha1 revision rather than a semver on PyPI because the change is
not released yet.

See also:

- google/yapf#1243

------

The core Triton is a small number of people, and we receive many PRs
(thank
you!).  To help us review your code more quickly, **if you are a new
contributor (less than 3 PRs merged) we ask that you complete the
following
tasks and include the filled-out checklist in your PR description.**

Complete the following tasks before sending your PR, and replace `[ ]`
with
`[x]` to indicate you have done them.

- [X] I am not making a trivial change, such as fixing a typo in a
comment.

- [X] I have written a PR description following these
  [rules](https://cbea.ms/git-commit/#why-not-how).

- [X] I have run `pre-commit run --from-ref origin/main --to-ref HEAD`.

- Select one of the following.
  - [ ] I have added tests.
    - `/test` for `lit` tests
    - `/unittest` for C++ tests
    - `/python/test` for end-to-end tests
  - [X] This PR does not need a test because `FILL THIS IN`.

- Select one of the following.
  - [x] I have not added any `lit` tests.
- [ ] The `lit` tests I have added follow these [best
practices](https://mlir.llvm.org/getting_started/TestingGuide/#filecheck-best-practices),
including the "tests should be minimal" section. (Usually running Python
code
    and using the instructions it generates is not minimal.)
Luosuu pushed a commit to Luosuu/triton that referenced this pull request Nov 13, 2024
@XuehaiPan @Luosuu
[CI] bump yapf version from be72557 to 7e21823 (triton-lang#4997)
e643d09 
This PR bumps `yapf` version from `be72557` to `7e21823` in pre-commit
hook. Commit `7e21823` fixes race condition when `pre-commit` running
`yapf` in parallel.

Use a sha1 revision rather than a semver on PyPI because the change is
not released yet.

See also:

- google/yapf#1243

------

The core Triton is a small number of people, and we receive many PRs
(thank
you!).  To help us review your code more quickly, **if you are a new
contributor (less than 3 PRs merged) we ask that you complete the
following
tasks and include the filled-out checklist in your PR description.**

Complete the following tasks before sending your PR, and replace `[ ]`
with
`[x]` to indicate you have done them.

- [X] I am not making a trivial change, such as fixing a typo in a
comment.

- [X] I have written a PR description following these
  [rules](https://cbea.ms/git-commit/#why-not-how).

- [X] I have run `pre-commit run --from-ref origin/main --to-ref HEAD`.

- Select one of the following.
  - [ ] I have added tests.
    - `/test` for `lit` tests
    - `/unittest` for C++ tests
    - `/python/test` for end-to-end tests
  - [X] This PR does not need a test because `FILL THIS IN`.

- Select one of the following.
  - [x] I have not added any `lit` tests.
- [ ] The `lit` tests I have added follow these [best
practices](https://mlir.llvm.org/getting_started/TestingGuide/#filecheck-best-practices),
including the "tests should be minimal" section. (Usually running Python
code
    and using the instructions it generates is not minimal.)
guacamoleo pushed a commit to guacamoleo/triton that referenced this pull request Nov 14, 2024
@XuehaiPan @guacamoleo
[CI] bump yapf version from be72557 to 7e21823 (triton-lang#4997)
6100121 
This PR bumps `yapf` version from `be72557` to `7e21823` in pre-commit
hook. Commit `7e21823` fixes race condition when `pre-commit` running
`yapf` in parallel.

Use a sha1 revision rather than a semver on PyPI because the change is
not released yet.

See also:

- google/yapf#1243

------

The core Triton is a small number of people, and we receive many PRs
(thank
you!).  To help us review your code more quickly, **if you are a new
contributor (less than 3 PRs merged) we ask that you complete the
following
tasks and include the filled-out checklist in your PR description.**

Complete the following tasks before sending your PR, and replace `[ ]`
with
`[x]` to indicate you have done them.

- [X] I am not making a trivial change, such as fixing a typo in a
comment.

- [X] I have written a PR description following these
  [rules](https://cbea.ms/git-commit/#why-not-how).

- [X] I have run `pre-commit run --from-ref origin/main --to-ref HEAD`.

- Select one of the following.
  - [ ] I have added tests.
    - `/test` for `lit` tests
    - `/unittest` for C++ tests
    - `/python/test` for end-to-end tests
  - [X] This PR does not need a test because `FILL THIS IN`.

- Select one of the following.
  - [x] I have not added any `lit` tests.
- [ ] The `lit` tests I have added follow these [best
practices](https://mlir.llvm.org/getting_started/TestingGuide/#filecheck-best-practices),
including the "tests should be minimal" section. (Usually running Python
code
    and using the instructions it generates is not minimal.)
@TheUbuntuGuy TheUbuntuGuy mentioned this pull request Jan 21, 2025
Closed
@anmyachev anmyachev mentioned this pull request Feb 5, 2025
Open
@hartwork hartwork deleted the fix-pickle-related-race-condition branch October 14, 2025 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bwendling bwendling bwendling approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

3 participants

@hartwork @a-gardner1 @bwendling