jrmccluskey commented on code in PR #40929:
URL: https://github.com/apache/beam/pull/40929#discussion_r4472922953
##########
sdks/python/apache_beam/ml/inference/utils.py:
##########
@@ -45,6 +46,17 @@ def _convert_to_result(
predictions: Union[Iterable, dict[Any, Iterable]],
model_id: Optional[str] = None,
) -> Iterable[PredictionResult]:
+ """Pairs each input in the batch with its prediction.
+
+ Predictions beyond the number of inputs are ignored.
+
+ Raises:
+ ValueError: if there are fewer predictions than inputs. Pairing stops at
+ the shorter of the two, so without this check the remaining inputs
+ would silently get no result.
+ """
+ if not isinstance(batch, Sized):
+ batch = list(batch)
Review Comment:
This extra check appears unrelated
##########
sdks/python/apache_beam/ml/inference/utils.py:
##########
@@ -53,11 +65,57 @@ def _convert_to_result(
predictions_per_tensor = [
dict(zip(predictions.keys(), v)) for v in zip(*predictions.values())
]
- return [
+ results = [
PredictionResult(x, y, model_id)
for x, y in zip(batch, predictions_per_tensor)
]
- return [PredictionResult(x, y, model_id) for x, y in zip(batch, predictions)]
+ else:
+ results = [
+ PredictionResult(x, y, model_id) for x, y in zip(batch, predictions)
+ ]
+ if len(results) < len(batch):
+ raise ValueError(
+ _missing_predictions_message(
+ len(batch), len(results), predictions, model_id))
+ return results
+
+
+def _count(number: int, singular: str, plural: str) -> str:
+ return f'{number} {singular if number == 1 else plural}'
+
+
+def _missing_predictions_message(
+ num_inputs: int,
+ num_predictions: int,
+ predictions: Union[Iterable, dict[Any, Iterable]],
+ model_id: Optional[str]) -> str:
+ expected = 'Expected one prediction per input'
+ if model_id:
+ expected += f' from model {model_id}'
+ inputs = _count(num_inputs, 'input', 'inputs')
+ if isinstance(predictions, dict):
+ short_outputs = []
+ for key, value in predictions.items():
+ try:
+ length = len(value)
+ except TypeError:
+ continue
+ if length < num_inputs:
+ short_outputs.append(
+ f'{key!r} has {_count(length, "entry", "entries")}')
+ if short_outputs:
+ outputs = 'output' if len(short_outputs) == 1 else 'outputs'
+ return (
+ f'{expected}, but {outputs} {", ".join(short_outputs)} for a batch '
+ f'of {inputs}. Every output must have one entry per input, so remove
'
+ 'any that do not (for example, pass inference_args={"use_cache": '
+ 'False} so that Hugging Face models do not return past_key_values), '
+ 'or use an inference function that returns one prediction per
input.')
+ predictions_count = _count(num_predictions, 'prediction', 'predictions')
+ return (
+ f'{expected}, but got {predictions_count} for a batch of {inputs}. If '
+ 'the model returns several outputs, such as a tuple, use an inference '
+ 'function that returns one prediction per input.')
Review Comment:
This is pretty over-engineered to raise a length mismatch here. Let's go for
something more concise and able to be in-lined in the `_convert_to_result()`
--
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]
