Fix functional::smooth_l1_loss signatures to not override beta

[kurtamohler]

This splits nn::functional::smooth_l1_loss into two different signatures in order to keep backward compatibility for calling the function like smooth_l1_loss(input, target, /*reduction=*/..., /*beta=*/...)

Fixes #70163

cc @ezyang @gchanan @albanD @mruberry @jbschlosser @walterddr @mikaylagawarecki

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/109798

• 📄 Preview Python docs built from this PR
• 📄 Preview C++ docs built from this PR
• ❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

Note: Links to docs will display an error until the docs builds have been completed.

✅ You can merge normally! (1 Unrelated Failure)

As of commit 38edd66 with merge base 733368a ():

FLAKY - The following job failed but was likely due to flakiness present on trunk:

• trunk / win-vs2019-cpu-py3 / test (default, 3, 3, windows.4xlarge.nonephemeral) (gh)

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[mikaylagawarecki]

LGTM

[mikaylagawarecki]

actually, should this be just const SmoothL1LossFuncOptions& options = {} to keep BC?

[kurtamohler]

smooth_l1_loss(..., const SmoothL1LossFuncOptions& options = {}, double beta) doesn't build because a default argument cannot be followed by a non-default argument.

smooth_l1_loss(..., const SmoothL1LossFuncOptions& options, double beta) does build, but it's semantically ambiguous because beta is given in two places. For instance, what should happen if it's called like smooth_l1_loss(input, target, SmoothL1LossFuncOptions().beta(0.5), /*beta=*/0.1)?

With the signatures in this PR, it's no longer possible to call the function in an ambiguous way. In that sense, it is BC-breaking. However, I would think that BC does not actually matter for ambiguous cases. Maybe I'm mistaken though

We do keep BC for the non-ambiguous case smooth_l1_loss(input, target, /*reduction=*/..., /*beta=*/...). There is an existing test for this in test/cpp/api/functional.cpp

[kurtamohler]

Although if we do break BC like this to remove the ambiguous case, maybe we would still need a deprecation cycle. torch_extensions builds in CI are failing from my change:

2023-09-21T18:54:43.9035006Z /var/lib/jenkins/.cache/torch_extensions/py311_cpu/functional_impl_check/main.cpp:1900:21: error: no matching function for call to 'smooth_l1_loss'
2023-09-21T18:54:43.9035367Z   auto cpp_output = F::smooth_l1_loss(
2023-09-21T18:54:43.9035584Z                     ^~~~~~~~~~~~~~~~~
2023-09-21T18:54:43.9036468Z /opt/conda/envs/py_3.11/lib/python3.11/site-packages/torch/include/torch/csrc/api/include/torch/nn/functional/loss.h:400:15: note: candidate function not viable: no known conversion from 'torch::nn::SmoothL1LossOptions' to 'SmoothL1LossFuncOptions::reduction_t' (aka 'variant<enumtype::kNone, enumtype::kMean, enumtype::kSum>') for 3rd argument
2023-09-21T18:54:43.9037033Z inline Tensor smooth_l1_loss(

Perhaps a better strategy is to go with smooth_l1_loss(..., const SmoothL1LossFuncOptions& options, double beta) after all, but just error out in ambiguous case where both beta parameters are set like smooth_l1_loss(input, target, SmoothL1LossFuncOptions().beta(0.5), /*beta=*/0.1). This should make the failing builds pass again. In order to do this though, we would need to change the beta field in SmoothL1LossFuncOptions from double to optional<double>, so we can detect if it was explicitly set or not. Is this alright with you?

This is still technically BC-breaking in the ambiguous case, but that case has to be broken in order to address the issue

[kurtamohler]

Perhaps a better strategy is to go with smooth_l1_loss(..., const SmoothL1LossFuncOptions& options, double beta) after all, but just error out in ambiguous case where both beta parameters are set like smooth_l1_loss(input, target, SmoothL1LossFuncOptions().beta(0.5), /*beta=*/0.1). This should make the failing builds pass again. In order to do this though, we would need to change the beta field in SmoothL1LossFuncOptions from double to optional<double>, so we can detect if it was explicitly set or not.

I've changed the PR to do this

[kurtamohler]

@mikaylagawarecki, are these changes alright with you?

[mikaylagawarecki]

Apologies for the delay, I think the BC-break for the C++ API for the type of beta is ok in this case as it is necessary to resolve the issue of beta being passed in both the options argument and as its own argument which is an unfortunate bug

[kurtamohler]

@pytorchbot merge

[pytorchmergebot]

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here