Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support hardswish function #100

Merged

Conversation

shaharelys
Copy link
Contributor

@shaharelys shaharelys commented Mar 28, 2024

Before submitting

What does this PR do?

Implements the HardSwish activation function.

Fixes #92

PR review

This PR is open for review, though I'm not entirely certain it's 100% complete, as this is my first contribution. All tests have passed, but docs has not been updated. Feedback from the community is welcomed!

Did you have fun?

Yes! 🙃

@t-vi
Copy link
Collaborator

t-vi commented Mar 29, 2024

@shaharelys Thank you, so excited to see you with a PR here.

@shaharelys
Copy link
Contributor Author

Hey guys! @nikitaved @t-vi, I would love to get your take on this whenever possible. In the meantime, I'll start working on the one_hot issue at #64.

@t-vi
Copy link
Collaborator

t-vi commented Apr 1, 2024

@shaharelys, Apologies for the delay. The PR seems great at first sight, but I wanted to have @nikitaved take a more thorough look and we've hit a very long weekend with bank holidays both Friday and today.

@t-vi
Copy link
Collaborator

t-vi commented Apr 1, 2024

While you are waiting, could you look into how to only test for the floating point inputs (on cuda?):

FAILED thunder/tests/test_ops.py::test_core_vs_torch_consistency_hardswish_nvfuser_cuda_int64 - RuntimeError: "hardswish_cuda" not implemented for 'Long'
FAILED thunder/tests/test_ops.py::test_core_vs_torch_consistency_hardswish_nvfuser_cuda_int32 - RuntimeError: "hardswish_cuda" not implemented for 'Int'
FAILED thunder/tests/test_ops.py::test_core_vs_torch_consistency_hardswish_torch_cuda_int64 - RuntimeError: "hardswish_cuda" not implemented for 'Long'
FAILED thunder/tests/test_ops.py::test_core_vs_torch_consistency_hardswish_torch_cuda_int16 - RuntimeError: "hardswish_cuda" not implemented for 'Short'
FAILED thunder/tests/test_ops.py::test_core_vs_torch_consistency_hardswish_torch_cuda_int32 - RuntimeError: "hardswish_cuda" not implemented for 'Int'
FAILED thunder/tests/test_ops.py::test_core_vs_torch_consistency_hardswish_torch_cuda_uint8 - RuntimeError: "hardswish_cuda" not implemented for 'Byte'
FAILED thunder/tests/test_ops.py::test_core_vs_torch_consistency_hardswish_torch_cuda_int8 - RuntimeError: "hardswish_cuda" not implemented for 'Char'

@t-vi
Copy link
Collaborator

t-vi commented Apr 1, 2024

I usually try to run this locally with pytest-3 thunder/tests/test_ops.py -k hardswish or so.

@shaharelys
Copy link
Contributor Author

shaharelys commented Apr 1, 2024

Okay, I believe this issue can be addressed by updating the following:

    # PyTorch does not support CPU integer types hardswish
        DecorateInfo(
            pytest.mark.xfail,
            "test_core_vs_torch_consistency",
            dtypes=(datatypes.int16, datatypes.int32, datatypes.int64, datatypes.int8, datatypes.uint8),
            devicetypes=(devices.DeviceType.CPU,),
        ),

to:

    # PyTorch does not support integer types for both the CPU and CUDA hardswish
        DecorateInfo(
            pytest.mark.xfail,
            "test_core_vs_torch_consistency",
            dtypes=(datatypes.int16, datatypes.int32, datatypes.int64, datatypes.int8, datatypes.uint8),
        ),

I've made this adjustment, and the issue should now be resolved.

Btw @t-vi, how could I have identified this error on my own? I ran pytest thunder/tests/test_ops.py -k hardswish -v with pytest ==7.4.4 and only got 16 passed, 3 skipped, 2074 deselected, 6 xfailed, 1 xpassed, 1 warning

thunder/tests/opinfos.py Outdated Show resolved Hide resolved
@mruberry
Copy link
Collaborator

mruberry commented Apr 1, 2024

Looks pretty good, @shaharelys! I made a recommendation to limit the operation's supported dtypes. I think the operation isn't defined on complex types, and while defined on exact types they're not very important to support.

thunder/torch/__init__.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@mruberry mruberry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @shaharelys! Looks great. The CI failure is an unrelated flaky failure.

@mruberry mruberry enabled auto-merge (squash) April 1, 2024 18:20
@shaharelys
Copy link
Contributor Author

Super excited about this! @mruberry Thanks for the support! Will now move back to the one_hot at #64

@mruberry mruberry merged commit 2bb3b60 into Lightning-AI:main Apr 2, 2024
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Operator support for F.hardswish
4 participants