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.

Sign up for GitHub

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

Jump to bottom

fix certain matchers breaking under multiprocessing by initializing them late #1204

Merged
merged 3 commits into from
Sep 25, 2024
Merged

fix certain matchers breaking under multiprocessing by initializing them late #1204

merged 3 commits into from
Sep 25, 2024

Conversation

kiri11
Copy link
Contributor

@kiri11 kiri11 commented Sep 2, 2024 •
edited
Loading

Support matcher decorators with multiprocessing on Windows/macOS.

See issue #1181 for more context.

Summary

  • Call _gather_matchers later in the code (first time when it's needed) to ensure we initialize them in a process AFTER the "fork".
  • Proactively check if attributes are not properties instead of trying to evaluate them and suppressing all errors.

Test Plan

Before

> hatch run python -m unittest libcst.codemod.tests.test_codemod_cli.TestCodemodCLI.test_matcher_decorators_multiprocessing

FAILED (failures=1)

After

> hatch run python -m unittest libcst.codemod.tests.test_codemod_cli.TestCodemodCLI.test_matcher_decorators_multiprocessing     
.
----------------------------------------------------------------------
Ran 1 test in 0.973s

OK

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 2, 2024
@kiri11 kiri11 force-pushed the multiprocessing-decorators branch 2 times, most recently from 85f1e0f to 6a29b07 Compare September 3, 2024 04:05
@kiri11 kiri11 changed the title Delayed initialization of matchers, fixes MatcherDecoratableTransformer multiprocessing bug Gather matchers later, fixes MatcherDecoratableTransformer multiprocessing bug Sep 3, 2024
zsol
zsol reviewed Sep 3, 2024
View reviewed changes
libcst/matchers/_visitors.py Outdated
@@ -366,6 +372,8 @@ def _visit_matchers(
node: cst.CSTNode,
metadata_resolver: cst.MetadataDependent,
) -> Dict[BaseMatcherNode, Optional[cst.CSTNode]]:
# Populate matchers dict if it's empty
matchers = matchers or _gather_matchers(metadata_resolver)
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we'll pay extra on every on_visit() now if there are no matchers declared in the visitor/transformer. Would be worth eliminating that

@kiri11
Add is_property check
8326c04 
Skip properties to prevent exceptions
@kiri11 kiri11 force-pushed the multiprocessing-decorators branch 2 times, most recently from 7cf829b to 14f4de2 Compare September 4, 2024 04:27
@kiri11 kiri11 marked this pull request as ready for review September 5, 2024 00:04
@kiri11 kiri11 requested a review from zsol September 5, 2024 00:04
zsol
zsol approved these changes Sep 25, 2024
View reviewed changes
Copy link
Member

@zsol zsol left a comment

Choose a reason for hiding this comment

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

Thank you!

This was linked to issues Sep 25, 2024
[Windows, Mac] Can't use call_if_inside/call_if_not_inside matcher decorators with multiprocessing #1181
Closed
Multiprocessing-only matchers bug on convert_type_comments #629
Closed
@zsol zsol changed the title Gather matchers later, fixes MatcherDecoratableTransformer multiprocessing bug fix certain matchers breaking under multiprocessing by gathering them late Sep 25, 2024
@zsol zsol changed the title fix certain matchers breaking under multiprocessing by gathering them late fix certain matchers breaking under multiprocessing by initializing them late Sep 25, 2024
@zsol zsol merged commit 9fd67bc into Instagram:main Sep 25, 2024
20 checks passed
This was referenced Sep 25, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zsol zsol zsol approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
3 participants
@kiri11 @zsol @facebook-github-bot