[core] Enable EventEngine for Python by default, and EventEngine fork support in Python and Ruby#41432
[core] Enable EventEngine for Python by default, and EventEngine fork support in Python and Ruby#41432murgatroid99 wants to merge 5 commits intogrpc:masterfrom
Conversation
Summary of ChangesHello @murgatroid99, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enables the EventEngine poller for Python by default, a crucial step in accelerating the migration to the new EventEngine architecture. By making this experiment opt-out rather than opt-in, the goal is to encourage broader adoption and uncover potential issues more rapidly, building on the success of previous external user tests. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request enables the event_engine_poller_for_python experiment by default. This is achieved by updating src/core/lib/experiments/rollouts.yaml, which consequently updates the auto-generated files src/core/lib/experiments/experiments.cc and src/core/lib/experiments/experiments.h. This change will cause gRPC for Python to use the EventEngine-based poller. The changes are correct and consistent with the goal of enabling the experiment for wider testing. I've reviewed the configuration changes and they appear to be correctly implemented without any issues.
sergiitk
left a comment
There was a problem hiding this comment.
Before we enable this by default, we need to add the CI coverage on macOS and make sure it's passing.
|
I modified this to also roll out the |
|
@asheshvidyut strange. After re-running Bazel Python MacOS Tests on the new pool, I see a whole bunch of test failures like so: |
sergiitk
left a comment
There was a problem hiding this comment.
LGTM, but we should look into Polling FD from a wrong generation error logs captured in #41432 (comment).
| default: true | ||
| - name: event_engine_fork | ||
| default: false | ||
| default: true |
There was a problem hiding this comment.
Please correct me if I'm wrong: this doesn't enable fork just for python, AFAIK this applies to all other wrapped language too (including ruby)
There was a problem hiding this comment.
If I'm correct, we should capture this in the PR title.
There was a problem hiding this comment.
As I said in #41432 (comment)
All code guarded by the
event_engine_forkexperiment is also guarded by theGRPC_ENABLE_FORK_SUPPORTcompilation flag, which only Python sets (as far as I can tell), so rolling this experiment out won't affect any other language.
There was a problem hiding this comment.
I have realized that I made a mistake in my previous searches, and Ruby does in fact also set the GRPC_ENABLE_FORK_SUPPORT compilation flag, here. I have updated the PR title to include that information.
|
Based on manual testing with the provided reproduction code, enabling this experiment for Ruby appears to fix #41264. |
… support in Python and Ruby (grpc#41432) I think the only way we can realistically make progress with this migration is by having people start using it. grpc#37710 (comment) shows that external users' tests can pass with this experiment enabled. <!-- If you know who should review your pull request, please assign it to that person, otherwise the pull request would get assigned randomly. If your pull request is for a specific language, please add the appropriate lang label. --> Closes grpc#41432 COPYBARA_INTEGRATE_REVIEW=grpc#41432 from murgatroid99:python_enable_event_engine2 af0f0a5 PiperOrigin-RevId: 874842538
… support in Python and Ruby (grpc#41432) I think the only way we can realistically make progress with this migration is by having people start using it. grpc#37710 (comment) shows that external users' tests can pass with this experiment enabled. <!-- If you know who should review your pull request, please assign it to that person, otherwise the pull request would get assigned randomly. If your pull request is for a specific language, please add the appropriate lang label. --> Closes grpc#41432 COPYBARA_INTEGRATE_REVIEW=grpc#41432 from murgatroid99:python_enable_event_engine2 af0f0a5 PiperOrigin-RevId: 874842538
I think the only way we can realistically make progress with this migration is by having people start using it.
#37710 (comment) shows that external users' tests can pass with this experiment enabled.