Add drop_example flag to the RunInference and Model Handler #23266

AnandInguva · 2022-09-15T23:05:51Z

Added a flag drop_example. This can be passed to RunInference API to drop the example from the PredictionResult.

if drop_example is True, prediction_result.example would be None

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

Choose reviewer(s) and mention them in a comment (R: @username).
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)

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

AnandInguva · 2022-09-15T23:11:23Z

Run Python 3.9 PostCommit

codecov · 2022-09-16T00:13:17Z

Codecov Report

Merging #23266 (8ccf608) into master (a8ca305) will decrease coverage by 0.13%.
The diff coverage is 55.00%.

@@            Coverage Diff             @@
##           master   #23266      +/-   ##
==========================================
- Coverage   73.59%   73.46%   -0.14%     
==========================================
  Files         716      718       +2     
  Lines       95338    95528     +190     
==========================================
+ Hits        70162    70177      +15     
- Misses      23880    24055     +175     
  Partials     1296     1296

Flag	Coverage Δ
python	`83.19% <55.00%> (-0.22%)`	⬇️

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

Impacted Files	Coverage Δ
...thon/apache_beam/ml/inference/pytorch_inference.py	`0.00% <0.00%> (ø)`
...hon/apache_beam/ml/inference/tensorrt_inference.py	`0.00% <0.00%> (ø)`
sdks/python/apache_beam/ml/inference/base.py	`95.83% <100.00%> (+0.18%)`	⬆️
...thon/apache_beam/ml/inference/sklearn_inference.py	`95.45% <100.00%> (-0.33%)`	⬇️
...am/examples/inference/tensorrt_object_detection.py	`0.00% <0.00%> (ø)`
...ks/python/apache_beam/runners/worker/sdk_worker.py	`88.94% <0.00%> (+0.31%)`	⬆️
sdks/python/apache_beam/io/localfilesystem.py	`91.72% <0.00%> (+0.75%)`	⬆️
...ks/python/apache_beam/runners/worker/data_plane.py	`89.26% <0.00%> (+1.69%)`	⬆️
...python/apache_beam/runners/worker/worker_status.py	`76.66% <0.00%> (+2.00%)`	⬆️

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

github-actions · 2022-09-16T02:07:53Z

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @yeandy for label python.

Available commands:

stop reviewer notifications - opt out of the automated review tooling
remind me after tests pass - tag the comment author after tests pass
waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

AnandInguva · 2022-09-16T02:54:17Z

One more way to do this is to pass the flag through the ModelHandler. The reason why I added it in the RunInference because RunInference emits PredictionResult and I thought it would be the appropriate place to control how the output[prediction result] should look.

damccorm · 2022-09-16T13:24:39Z

One more way to do this is to pass the flag through the ModelHandler. The reason why I added it in the RunInference because RunInference emits PredictionResult and I thought it would be the appropriate place to control how the output[prediction result] should look.

I agree

sdks/python/apache_beam/ml/inference/base.py

sdks/python/apache_beam/ml/inference/base_test.py

sdks/python/apache_beam/ml/inference/sklearn_inference.py

yeandy

If this gets merged before TensorRT, we'll need to remember to rebase that.

yeandy · 2022-09-16T14:34:59Z

sdks/python/apache_beam/ml/inference/base.py

    """Runs inferences on a batch of examples.

    Args:
      batch: A sequence of examples or features.
      model: The model used to make inferences.
      inference_args: Extra arguments for models whose inference call requires
        extra parameters.
+        drop_example: Enable this to drop the example from PredictionResult


Suggested change

drop_example: Enable this to drop the example from PredictionResult

drop_example: Boolean flag indicating whether or not to drop the example from PredictionResult

sdks/python/apache_beam/ml/inference/base.py

sdks/python/apache_beam/ml/inference/pytorch_inference.py

sdks/python/apache_beam/ml/inference/sklearn_inference.py

yeandy · 2022-09-16T14:47:17Z

sdks/python/apache_beam/ml/inference/base_test.py

+
+    pipeline = TestPipeline()
+    examples = [1, 3, 5]
+    model_handler = FakeModelHandlerWithPredictionResult()


Shouldn't this have drop_example=True?

We are passing drop_example via base.RunInference, which passes it to the ModelHandler.

More explanation: #23266 (comment)

Thanks! I missed seeing the drop=True in the test example 😄

sdks/python/apache_beam/ml/inference/base_test.py

AnandInguva · 2022-09-16T15:08:53Z

PTAL @yeandy @damccorm

yeandy

LGTM

github-actions · 2022-09-16T15:39:25Z

R: @pabloem for final approval

damccorm · 2022-09-16T15:44:17Z

If this gets merged before TensorRT, we'll need to remember to rebase that.

I'd vote we let that one merge first since it is the higher complexity change (and less easy for us to control). Its currently just blocked on Python PreCommit flakes I think

damccorm

I know it came in after this PR, but could you please extend these changes to tensorrt_inference.py as well?

AnandInguva · 2022-09-16T17:58:46Z

Run Python 3.8 PostCommit

AnandInguva · 2022-09-16T17:59:03Z

stop reviewer notifications

damccorm · 2022-09-16T18:07:07Z

sdks/python/apache_beam/ml/inference/tensorrt_inference.py

@@ -272,7 +273,8 @@ def run_inference(

      return [
          PredictionResult(
-              x, [prediction[idx] for prediction in cpu_allocations]) for idx,
+              x if not drop_example else None,
+              [prediction[idx] for prediction in cpu_allocations]) for idx,


Can we use _convert_to_result here too? I don't think there's a reason these predictions can't be dictionaries, so this will handle that case automatically

github-actions · 2022-09-16T18:19:30Z

Stopping reviewer notifications for this pull request: requested by reviewer

AnandInguva · 2022-09-16T18:23:30Z

sdks/python/apache_beam/ml/inference/tensorrt_inference.py

-              x, [prediction[idx] for prediction in cpu_allocations]) for idx,
-          x in enumerate(batch)
-      ]
+      predictions = []


PTA:L @damccorm I think this is how we use _convert_to_result

cc : @yeandy

damccorm · 2022-09-16T18:31:27Z

That looks right to me, lets run the PostCommit as well though

damccorm · 2022-09-16T18:31:31Z

Run Python 3.8 PostCommit

damccorm · 2022-09-16T19:41:50Z

@AnandInguva looks like there's a linting violation. If that is fixed and we have a successful postcommit run I think this should be good to merge.

AnandInguva · 2022-09-16T22:10:33Z

https://ci-beam.apache.org/job/beam_PostCommit_Python38_PR/603/console passes

damccorm · 2022-09-17T12:14:29Z

Run Python PreCommit

damccorm

LGTM - will merge once we get past this flaky precommit

…pache#23266)" This reverts commit f477b85.

…23266)" (#23392) This reverts commit f477b85.

github-actions bot added the python label Sep 15, 2022

Add drop_example flag to the RunInference and Model Handler

60435d8

AnandInguva force-pushed the optional-flag branch from 6978a63 to 60435d8 Compare September 15, 2022 23:49

github-actions bot added the Next Action: Reviewers label Sep 16, 2022

damccorm reviewed Sep 16, 2022

View reviewed changes

AnandInguva added 4 commits September 16, 2022 10:15

Pass drop_example to the _convert_to_result

a1d05f3

Refactor _convert_to_result

0d0758b

Return _convert_to_result

e39f472

Fixup lint

1c8fd4d

yeandy reviewed Sep 16, 2022

View reviewed changes

AnandInguva added 2 commits September 16, 2022 10:58

Code update based on Suggestions

c3bea3e

Refactor class name

615c2b7

yeandy approved these changes Sep 16, 2022

View reviewed changes

damccorm reviewed Sep 16, 2022

View reviewed changes

AnandInguva added 3 commits September 16, 2022 13:27

Merge remote-tracking branch 'upstream/master' into optional-flag

aa06472

Refactor TensorRT to add drop_example

b3e8a08

Add typing.Optional to the type hint for drop_example

f41d745

damccorm reviewed Sep 16, 2022

View reviewed changes

Add _convert_to_result for tensorRT run_inference

9428bd5

AnandInguva commented Sep 16, 2022

View reviewed changes

fixup lint

8ccf608

damccorm approved these changes Sep 17, 2022

View reviewed changes

damccorm merged commit f477b85 into apache:master Sep 18, 2022

AnandInguva added a commit to AnandInguva/beam that referenced this pull request Sep 28, 2022

Revert "Add drop_example flag to the RunInference and Model Handler (a…

cda3a60

…pache#23266)" This reverts commit f477b85.

AnandInguva mentioned this pull request Sep 28, 2022

Revert "Add drop_example flag to the RunInference and Model Handler" #23392

Merged

damccorm pushed a commit that referenced this pull request Oct 4, 2022

Revert "Add drop_example flag to the RunInference and Model Handler (#…

72237d6

…23266)" (#23392) This reverts commit f477b85.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add drop_example flag to the RunInference and Model Handler #23266

Add drop_example flag to the RunInference and Model Handler #23266

AnandInguva commented Sep 15, 2022 •

edited

Loading

AnandInguva commented Sep 15, 2022

codecov bot commented Sep 16, 2022 •

edited

Loading

github-actions bot commented Sep 16, 2022

AnandInguva commented Sep 16, 2022

damccorm commented Sep 16, 2022

yeandy left a comment

yeandy Sep 16, 2022

yeandy Sep 16, 2022

AnandInguva Sep 16, 2022

yeandy Sep 16, 2022

AnandInguva commented Sep 16, 2022

yeandy left a comment

github-actions bot commented Sep 16, 2022

damccorm commented Sep 16, 2022

damccorm left a comment

AnandInguva commented Sep 16, 2022

AnandInguva commented Sep 16, 2022

damccorm Sep 16, 2022

github-actions bot commented Sep 16, 2022

AnandInguva Sep 16, 2022

AnandInguva Sep 16, 2022

damccorm commented Sep 16, 2022

damccorm commented Sep 16, 2022

damccorm commented Sep 16, 2022

AnandInguva commented Sep 16, 2022

damccorm commented Sep 17, 2022

damccorm left a comment

	drop_example: Enable this to drop the example from PredictionResult
	drop_example: Boolean flag indicating whether or not to drop the example from PredictionResult

Add drop_example flag to the RunInference and Model Handler #23266

Add drop_example flag to the RunInference and Model Handler #23266

Conversation

AnandInguva commented Sep 15, 2022 • edited Loading

GitHub Actions Tests Status (on master branch)

AnandInguva commented Sep 15, 2022

codecov bot commented Sep 16, 2022 • edited Loading

Codecov Report

github-actions bot commented Sep 16, 2022

AnandInguva commented Sep 16, 2022

damccorm commented Sep 16, 2022

yeandy left a comment

Choose a reason for hiding this comment

yeandy Sep 16, 2022

Choose a reason for hiding this comment

yeandy Sep 16, 2022

Choose a reason for hiding this comment

AnandInguva Sep 16, 2022

Choose a reason for hiding this comment

yeandy Sep 16, 2022

Choose a reason for hiding this comment

AnandInguva commented Sep 16, 2022

yeandy left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 16, 2022

damccorm commented Sep 16, 2022

damccorm left a comment

Choose a reason for hiding this comment

AnandInguva commented Sep 16, 2022

AnandInguva commented Sep 16, 2022

damccorm Sep 16, 2022

Choose a reason for hiding this comment

github-actions bot commented Sep 16, 2022

AnandInguva Sep 16, 2022

Choose a reason for hiding this comment

AnandInguva Sep 16, 2022

Choose a reason for hiding this comment

damccorm commented Sep 16, 2022

damccorm commented Sep 16, 2022

damccorm commented Sep 16, 2022

AnandInguva commented Sep 16, 2022

damccorm commented Sep 17, 2022

damccorm left a comment

Choose a reason for hiding this comment

AnandInguva commented Sep 15, 2022 •

edited

Loading

codecov bot commented Sep 16, 2022 •

edited

Loading