Only register cudnn executor if it is available

[KaelanDt]

Before submitting

• Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure to update the docs?
• Did you write any new necessary tests?

What does this PR do?

Fixes #2149. Also removes cudnn from the BaseRecipe, as it is not always there and is not a core component of thunder (less so than nvfuser).

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

[t-vi]

@riccardofelluga do the updates make you happier about the PR?

[t-vi]

Looks good to me, let's give @riccardofelluga @vedaanta a chance to look at it before merging it Wed or Thu.

[riccardofelluga]

Since we are modifying two files at this point wouldn't it be better to follow the same style as the other executors that are dependent on another library? Where we dedicate one file to be the gate for the symbols depending on the availability of the library(as in nvfuserex.py)

So something like cudnnex.py that takes care of the imports and checks and then cudnn_layernorm as is, and maybe creating cudnn_sdpa.py

@KaelanDt What do you think?

[KaelanDt]

Since we are modifying two files at this point wouldn't it be better to follow the same style as the other executors that are dependent on another library? Where we dedicate one file to be the gate for the symbols depending on the availability of the library(as in nvfuserex.py)

So something like cudnnex.py that takes care of the imports and checks and then cudnn_layernorm as is, and maybe creating cudnn_sdpa.py

@KaelanDt What do you think?

yes, i like that

[KaelanDt]

@riccardofelluga I tried a few things by adding a cudnn_sdpa file, but it just ended up being messy and relying on a bunch of cudnn_available checks and was more confusing in my opinion.

Since the cudnn_sdpa operators are registered along with the main cudnn executor, i think it is fine to leave it as is, and to have cudnn_layernorm as a separate file (because it creates a new executor)

[vedaanta]

the change looks good to me.

@KaelanDt @t-vi I think we should not register cudnn layernorm at all.
It was being bought up a year ago, but there were cpu overheads which made it perform worse than triton/nvfuser.
(Since then cudnn has worked a lot on norms, but their e2e contribution for models I know of has been low enough to not warrant enabling in thunder again.)

[t-vi]

Thank you @vedaanta !

review addressed

[riccardofelluga]

@riccardofelluga I tried a few things by adding a cudnn_sdpa file, but it just ended up being messy and relying on a bunch of cudnn_available checks and was more confusing in my opinion.

Since the cudnn_sdpa operators are registered along with the main cudnn executor, i think it is fine to leave it as is, and to have cudnn_layernorm as a separate file (because it creates a new executor)

@KaelanDt Let me come in help here, instead of modifying the cudnn executor and doing a conditional registration of the executor, wouldn't it be better to create a conditional import like in nvfuserex.py?

Would something like this in cudnnex.py work?

__all__ = ["cudnn_ex"]

cudnn_ex = None

if cudnn_available():
    import import thunder.executors.cudnn_spda as sdpa_impl
    cudnn_ex = sdpa_impl.cudnn_ex

else:
    warnings.warn("cudnn not available")

We can also make room for more complex import logic if needed (like say checking versions or disabling cudnn_linearnorm), all without modifying the cudnn executor registration.

[riccardofelluga]

Almost there!! Thanks for taking the time, I've added some suggestions. Please feel free to ping me if you need any help

[riccardofelluga]

Thanks for the changes, there are some nits but it looks good already!