Perplexity gpu mem optimization #345
Conversation
| @@ -47,11 +46,7 @@ def example_seq_gen_0(seed: int = 1234) -> Dict[str, Any]: | |||
| [ | |||
| ( | |||
| "perplexity", | |||
| CI( | |||
There was a problem hiding this comment.
Why did you remove that?
I see that the ground truth remained the same.. But now we don't cover CI() in the examples (i.e. test coverage)
There was a problem hiding this comment.
We do cover CI in other examples I hope.
I removed it because the perplexity now is computed at the batch level (more optimized) and I'm not sure it's a metric we would want a CI for.
We can support it back when necessary.
| target=target, | ||
| metric_per_batch_func=partial( | ||
| _perplexity_update, ignore_index=ignore_index | ||
| log_probs="log_probs", # collect log_probs - output of _perplexity_update |
There was a problem hiding this comment.
I think it's out of the scope of this PR but still sharing:
I don't like passing arbitrary key-values as **kwargs to the parent class where they have an actual purpose:
From MetricPerBatchDefault description:
:param kwargs: specify keywords and value arguments you want to collect from the source data.
can be strings (key names) and/or actual values
to collect from the results dictionary: add a "results:" prefix to the key name
I would rather see an argument like: key_values_to_collect: dict.
What's your take on that?
There was a problem hiding this comment.
I agree with you.
But it's indeed out of PR scope.
| # avoid from overflow | ||
| if preds.dtype == torch.float16: | ||
| preds = preds.to(torch.float32) | ||
| preds = torch.clamp(preds, min=1e-10) |
There was a problem hiding this comment.
Why do you clamp after moving from float16 to float32 ? Isn't float32 should be more stable? I would guess the other way around - clamp before moving from float32 into float16
There was a problem hiding this comment.
Plus, I think that values in (0, 1e-10) cannot be exist in float16, so why clamping that after moving to float32 ?
There was a problem hiding this comment.
I'm getting absolute zeros in float16 precision which causes log() to be infinity.
| @@ -106,6 +117,10 @@ def _perplexity_compute( | |||
| log_probs: List[np.ndarray], | |||
| token_num: List[np.ndarray], | |||
| ) -> float: | |||
| # avoid from overflow on large epochs | |||
| log_probs = [e.astype(np.float64) for e in log_probs] | |||
There was a problem hiding this comment.
numerical stability?
There was a problem hiding this comment.
It might be large numbers with many GPUS so I took the safe side here.
| def _df_dict_apply( | ||
| data: pd.Series, func: Callable, batch: bool = False | ||
| ) -> pd.Series: | ||
| if batch: |
There was a problem hiding this comment.
Not sure I understood the use-case: data is always a single data point (e.g. series) and func might be perform on a batch level? And where func expects a batch you pass batch=True?
I'm suggesting to rename batch or add a short description to make it clearer :))
There was a problem hiding this comment.
The metric has some preprocessing func in a batch level.
Here we are not running a training loop,, to avoid duplication, we run the function on each sample as if it was a batch. To do that we are converting single sample to batch, call the function and then squeezed it back to sample.
As I mentioned, those are dark places in fuse, that I would refactor If we had some time to spend on it.
No description provided.