gemini-code-assist[bot] commented on code in PR #38569:
URL: https://github.com/apache/beam/pull/38569#discussion_r3277240129
##########
sdks/python/apache_beam/testing/benchmarks/inference/table_row_inference_benchmark.py:
##########
@@ -72,32 +72,34 @@ def __init__(self):
metrics_namespace=self.metrics_namespace,
is_streaming=False,
pcollection='RunInference/BeamML_RunInference_Postprocess-0.out0')
- self.is_streaming = ((self.pipeline.get_option('mode') or
- 'batch') == 'streaming')
+ opts = self.pipeline.get_pipeline_options().view_as(
+ TableRowInferenceOptions)
+ mode = opts.mode or 'batch'
+ self.is_streaming = mode == 'streaming'
if self.is_streaming:
- self.subscription = self.pipeline.get_option('input_subscription')
+ self.subscription = opts.input_subscription
def test(self):
"""Execute the table row inference pipeline for benchmarking."""
- extra_opts = {}
-
- mode = self.pipeline.get_option('mode') or 'batch'
- extra_opts['mode'] = mode
+ opts = self.pipeline.get_pipeline_options().view_as(
+ TableRowInferenceOptions)
+ mode = opts.mode or 'batch'
+ extra_opts = {'mode': mode}
if mode == 'streaming':
- extra_opts['input_subscription'] = self.pipeline.get_option(
- 'input_subscription')
- extra_opts['window_size_sec'] = int(
- self.pipeline.get_option('window_size_sec') or 60)
- extra_opts['trigger_interval_sec'] = int(
- self.pipeline.get_option('trigger_interval_sec') or 30)
- else:
- extra_opts['input_file'] = self.pipeline.get_option('input_file')
-
- for opt in ['output_table', 'model_path', 'feature_columns']:
- val = self.pipeline.get_option(opt)
- if val:
- extra_opts[opt] = val
+ if opts.input_subscription:
+ extra_opts['input_subscription'] = opts.input_subscription
+ extra_opts['window_size_sec'] = opts.window_size_sec or 60
+ extra_opts['trigger_interval_sec'] = opts.trigger_interval_sec or 30
Review Comment:
Using the `or` operator for default values can lead to a logic regression if
`0` is a valid input. In the original code, `int(self.pipeline.get_option(...)
or 60)` would correctly preserve `0` because `get_option` returns a string
(e.g., "0"), which is truthy. Since `TableRowInferenceOptions` likely parses
these as integers, `0 or 60` will evaluate to `60`. It is safer to check for
`None` explicitly.
```suggestion
extra_opts['window_size_sec'] = opts.window_size_sec if
opts.window_size_sec is not None else 60
extra_opts['trigger_interval_sec'] = opts.trigger_interval_sec if
opts.trigger_interval_sec is not None else 30
```
##########
sdks/python/apache_beam/testing/benchmarks/inference/table_row_inference_benchmark.py:
##########
@@ -72,32 +72,34 @@ def __init__(self):
metrics_namespace=self.metrics_namespace,
is_streaming=False,
pcollection='RunInference/BeamML_RunInference_Postprocess-0.out0')
- self.is_streaming = ((self.pipeline.get_option('mode') or
- 'batch') == 'streaming')
+ opts = self.pipeline.get_pipeline_options().view_as(
+ TableRowInferenceOptions)
+ mode = opts.mode or 'batch'
+ self.is_streaming = mode == 'streaming'
if self.is_streaming:
- self.subscription = self.pipeline.get_option('input_subscription')
+ self.subscription = opts.input_subscription
def test(self):
"""Execute the table row inference pipeline for benchmarking."""
- extra_opts = {}
-
- mode = self.pipeline.get_option('mode') or 'batch'
- extra_opts['mode'] = mode
+ opts = self.pipeline.get_pipeline_options().view_as(
+ TableRowInferenceOptions)
Review Comment:
The pipeline options are already retrieved and viewed as
`TableRowInferenceOptions` in the `__init__` method. Consider storing the
options object as an attribute (e.g., `self.opts`) during initialization to
avoid re-parsing and redundant code in the `test` method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
