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

Force users to always specify a tag when creating a MultiProcessShared object (breaking) #26178

Merged
merged 3 commits into from
Apr 25, 2023

Conversation

damccorm
Copy link
Contributor

@damccorm damccorm commented Apr 7, 2023

Right now, we don't require a value for the mps tag. This means that the default behavior (if a user doesn't know what they're doing) is to not actually share the object across processes; instead each process will create an object for that process.

One downside of this is that this change does make it harder to have a single process that proxies all requests to a separate process (which is the current behavior if you specify no tag and alwaysProxy=true). If a user really wants that use case, they can do something like abc=MultiProcessShared(<constructor>, tag=uuid.uuid().hex, alwaysProxy=true) to get the same behavior. I think that will be a much rarer use case though.

This change is intentionally breaking because if anyone is currently using this feature they're probably actually intending to share the object. Plus this whole thing was sorta busted in the mainline case anyways (see #26172)


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests
Go tests

See CI.md for more information about GitHub Actions CI.

@github-actions github-actions bot added the python label Apr 7, 2023
@damccorm
Copy link
Contributor Author

damccorm commented Apr 7, 2023

Looks like tests are going to fail until we have https://github.com/apache/beam/actions/runs/4640195969/jobs/8211875305?pr=26178 (which is good evidence that we need it 😃)

@codecov
Copy link

codecov bot commented Apr 7, 2023

Codecov Report

Merging #26178 (81eb884) into master (f549fd3) will decrease coverage by 9.29%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master   #26178      +/-   ##
==========================================
- Coverage   81.12%   71.83%   -9.29%     
==========================================
  Files         469      752     +283     
  Lines       67185   101419   +34234     
==========================================
+ Hits        54501    72851   +18350     
- Misses      12684    27086   +14402     
- Partials        0     1482    +1482     
Flag Coverage Δ
python 81.08% <94.11%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
sdks/go/pkg/beam/core/runtime/exec/status.go 0.00% <ø> (ø)
...kg/beam/core/runtime/graphx/schema/logicaltypes.go 69.23% <ø> (ø)
...s/go/pkg/beam/core/runtime/graphx/schema/schema.go 65.94% <55.55%> (ø)
sdks/go/pkg/beam/core/runtime/exec/plan.go 70.00% <56.25%> (ø)
...s/python/apache_beam/utils/multi_process_shared.py 96.92% <94.11%> (-0.52%) ⬇️

... and 293 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@damccorm damccorm marked this pull request as ready for review April 17, 2023 21:31
@damccorm
Copy link
Contributor Author

R: @robertwb

@github-actions
Copy link
Contributor

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

@damccorm
Copy link
Contributor Author

Note that merging in master with #26172 fixed the previously failing test

@damccorm damccorm requested a review from robertwb April 18, 2023 20:48
@damccorm damccorm merged commit 73ce3b6 into apache:master Apr 25, 2023
@damccorm damccorm deleted the users/damccorm/mps-tags branch April 25, 2023 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants