Fix performance issue with sample weights in model.fit()

[nershman]

I previously had a PR open for this but I guess it got automatically closed when I reverted my commits...

Previous PR: #16177

@gbaned
@fchollet Since the way DataAdapter works is not clear to me I went back to training_utils.handle_partial_sample_weights.

The function is being passed a tensor when it should be passed a list. I think we can simply add a typecheck and if a tensor is passed then we wrap it in a list. This will fix both the slowdown as well as make sure the functions is checking that sample_weights correspond to inputs and outputs instead of checking every single sample in the tensor.

i.e.

if not isinstance(sample_weights, (list, tuple)):
    sample_weights = (sample_weights,)

And this will work fine, when the [sample_weights] workaround is used in model.fit() this is exactly what it does, it causes a tuple of one tensor to be passed to the function instead of just a tensor.
How is that?

[haifeng-jin]

Pending on another reply from @fchollet .
Hold till 01/09/2023 to add the pending label again if no response.

[fchollet]

LGTM, thanks

[fchollet]

Unfortunately I'm not able to merge as I've seeing a lot of test failures:

keras/engine:data_adapter_test
keras/engine:data_adapter_test
keras/engine:training_test
keras/engine:training_test
keras/metrics:metrics_correctness_test
keras/metrics:metrics_correctness_test
keras/tests:temporal_sample_weights_correctness_test
keras/tests:temporal_sample_weights_correctness_test

Can you take a look?

[haifeng-jin]

@nershman Would you please take a look at the test failures?
Thanks!

[gbaned]

Hi @nershman Can you please check @haifeng-jin's comments and keep us posted ? Thank you!

[gbaned]

Hi @nershman Any update on this PR? Please. Thank you!

[nershman]

Hi @nershman Any update on this PR? Please. Thank you!

Hi, I have been so busy with work recently, sorry. I have some notes on this and I'll look deeper into it this weekend.

[nershman]

Wrapping the weights was causing issues further down in the function. I just added a case to the partial sample check in the beginning instead.

    if not isinstance(sample_weights, (list, tuple)): 
        any_sample_weight = (sample_weights,) is not None and sample_weights is not None  #wrap the weights during check instead of overwriting
        partial_sample_weight = any_sample_weight and sample_weights is None
    else: #normal check
        any_sample_weight = sample_weights is not None and any(
            w is not None for w in sample_weights
        )
        partial_sample_weight = any_sample_weight and any(
            w is None for w in sample_weights
        )

Tests pass on my machine now. (data_adapter_test, training_test, metrics_correctness_test, temporal_sample_weights_correctness_test)

[haifeng-jin]

I am approving this PR to see if internal tests passes.

[chuckatkins]

Is it reasonable to try to have NumPy process the weights directly if possible, and in doing so give any non-finite weight the same treatment as None? Something like:

if sample_weights is None:
    any_sample_weight = False
    partial_sample_weight = False
else:
    try:
        sample_weights_isfinite = np.isfinite(sample_weights)
        any_sample_weight = np.any(sample_weights_isfinite)
        if any_sample_weight:
            partial_sample_weight = not np.all(sample_weights_isfinite)
            if partial_sample_weight:
                new_sample_weights = np.nan_to_num(sample_weights, nan=1.0, posinf=1.0, neginf=1.0)
                return new_sample_weights, any_sample_weight, partial_sample_weight
    except TypeError:
        if not isinstance(sample_weights, (list, tuple)):
            any_sample_weight = True
            partial_sample_weight = False
        else:
            any_sample_weight = any(w is not None for w in sample_weights)
            partial_sample_weight = any_sample_weight and any(w is None for w in sample_weights)

if not any_sample_weight:
    return None, any_sample_weight, partial_sample_weight

if not partial_sample_weight:
    return sample_weights, any_sample_weight, partial_sample_weight

[nershman]

@chuckatkins I think you should make a separate bug report for this, I'm not familiar with what you're trying to fix. But my concern with using numpy would be creating issues with eager execution.