Repository navigation
Scope RunInference model_identifier annotation to BeamML_RunInference - #40671
Conversation
8f9def8 to
ab4245c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40671 +/- ##
==========================================
+ Coverage 80.04% 80.07% +0.02%
==========================================
Files 490 490
Lines 82916 83005 +89
==========================================
+ Hits 66373 66465 +92
+ Misses 16543 16540 -3
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:
|
|
R: @tvalentyn |
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
AMOOOMA
changed the title
7ddd3f0 to
93cfdef
Compare
93cfdef to
c11ecad
Compare
| batched_elements_pcoll | ||
| | 'BeamML_RunInference' >> run_inference_pardo) | ||
| with batched_elements_pcoll.pipeline.transform_annotations( | ||
| model_identifier=self._model_tag): |
There was a problem hiding this comment.
do we want to add this annotation even when not using ModelManager, or should we downscope this ?
There was a problem hiding this comment.
based on offline discussion sounds like it is probably ok to keep as is.
| run_inference_pardo.with_exception_handling( | ||
| exc_class=self._exc_class, | ||
| use_subprocess=self._use_subprocess, | ||
| threshold=self._threshold, |
There was a problem hiding this comment.
it appears that if threshold < 1, then we add a few more nodes in the pipeline graph that will unintentionally get this annotation, see:
beam/sdks/python/apache_beam/transforms/core.py
Line 2414 in 5b6aa70
I don't have an opinion yet how to best account for this.
There was a problem hiding this comment.
Good catch, I agree we should probably refactor this. I will add a comment for now and make a feature request for the refactor and follow up to keep this change smaller.
There was a problem hiding this comment.
possible direction to consider (note: this is a draft rn #40738)
| self.assertTrue(len(outlet_transforms) > 0) | ||
| for t in outlet_transforms: | ||
| self.assertEqual(t.annotations.get("model_identifier"), b"") | ||
| self.assertFalse(t.annotations.get("model_identifier")) |
There was a problem hiding this comment.
should we remove
?There was a problem hiding this comment.
Ah yes we should, that got lost in the split, will add it back.
… add TODO(apache#40715) for threshold transforms
7fab7c7 to
1e0b941
Compare
Summary
Previously,
RunInference.annotations()attachedmodel_identifierto the outerRunInferencecompositePTransform, which caused all nested sub-transforms (such asWatchFilePatternside-input polling transforms,BatchElements,BeamML_RunInference_MonitoringOutlet, preprocess/postprocess transforms, etc.) to inherit themodel_identifierannotation. Runners that group steps bymodel_identifierto look upnum_loaded_models_<model_identifier>would then attachmodel_identifierto steps that never execute_RunInferenceDoFn.This PR removes
model_identifierfromRunInference.annotations()and attachestransform_annotations(model_identifier=self._model_tag)directly aroundBeamML_RunInferenceinRunInference.expand().Testing
test_run_inference_only_attaches_model_identifier_to_inference_stepinbase_test.py.test_run_inference_monitoring_outlet_shadows_model_identifierinvertex_ai_model_monitoring_v2_test.py.