Repository navigation
Remove usage of (leaking) shared handles, which can also accidentally release the last reference holding the model. - #40441
Merged
Conversation
…ly release the last reference hodling a model in _SharedMap._keepalive when all DoFns have been gc'ed.
Contributor
|
Assigning reviewers: R: @damccorm for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40441 +/- ##
=========================================
Coverage 59.04% 59.04%
Complexity 15624 15624
=========================================
Files 2797 2797
Lines 280750 280789 +39
Branches 12488 12488
=========================================
+ Hits 165764 165795 +31
- Misses 108540 108548 +8
Partials 6446 6446
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RunInference framework uses _ModelStatus objects to store a state of the model, like whether the model is valid or should be reloaded. We need to store one _ModelStatus object per one instance of RunInference. SInce there can be more than one such object, we key them by a tag:
{model_tag}_model_status.To share these objects within the SDK process, we rely on
shared.Shared(), which also has a concept of tags and is a tool to share a singleton object across multiple DoFn instances.However, the usage
return shared.Shared().acquire(lambda: _ModelStatus(False), tag=tag)is incorrect since this creates a new shared handle every time get_model_status is called. The subsequentacquirecall, creates a new, uninitialized _ModelStatus instance.This confusion is likely caused by similar Beam APIs:
sharedvsmultiprocess_shared, which use tags slightly differently.For MultiProcessShared, tag is the key. The same tag from any process gives the same object.
For shared.Shared, it is a version label. The key is the handle's own internal uuid. Acquiring through the same handle with a different tag throws away the old object and builds a new one; the same tag through a new handle also gives a new object.
The
Sharedhandle is generally not expected to be created more than once per stage of the pipeline (recall that this was originally written for Batch pipelines in TFX; with streaming it gets more complicated since we have all stages running at once).Given that a) RunInference used 2 shared handles, one for the model, one for the ModelStatus, and b) the Shared machinery has a single
_SharedMap._keepaliveslot for entire process, callingload_model_statusinvalidated the last existing reference to the model in some cases, causing #40440.Because we instantiated a new ModelStatus object every time, we didn't store the state of previously loaded models properly, so invalidating a model might have not caused the reload. However, this would only happen if we are NOT sharing the model across processes, so impact should be limited.
fixes: #40440