Handling <unsupported/supported> <dypes/devices> for mixed functions

[hello-fri-end]

• Trello: Fix recurse option in ivy.function_supported_devices_and_dtypes and ivy.function_unsupported_devices_and_dtypes
• Trello: Investigate why _get_supported_devices_dtypes function is not returning the correct dtype set for mixed functions

[hello-fri-end]

let me know once you're done with the changes, as I suppose this review request may not be intended. Thanks @hello-fri-end 🙂

oh, all done here actually 😅

[hello-fri-end]

1. could make the implementation a bit cleaner, because the intersection of unsupported dtypes for the backend-specific implementation and the compositional implementation wouldn't necessarily be the correct set of unsupported dtypes.

Thanks for the detailed review @VedPatwardhan :rocket: Completely agree with cleaning up the implementation part - will add a private helper function that all of these functions can use. I also agree that returning a tuple containing the tuple of unsupported dtypes for the backend-specific implementation and the tuple of unsupported dtypes for the compositional implementation makes much more sense, thanks for pointing this out :+1: . In this case though, how should I update the handle_test decorator which expects that this function returns only one tuple containing the datatypes that are supported by the function to be tested ?

[hello-fri-end]

1. could make the implementation a bit cleaner, because the intersection of unsupported dtypes for the backend-specific implementation and the compositional implementation wouldn't necessarily be the correct set of unsupported dtypes.

Thanks for the detailed review @VedPatwardhan rocket Completely agree with cleaning up the implementation part - will add a private helper function that all of these functions can use. I also agree that returning a tuple containing the tuple of unsupported dtypes for the backend-specific implementation and the tuple of unsupported dtypes for the compositional implementation makes much more sense, thanks for pointing this out +1 . In this case though, how should I update the handle_test decorator which expects that this function returns only one tuple containing the datatypes that are supported by the function to be tested ?

That's a great point! I think it would be a bit tricky in the tests (I'm also pinging @CatB1t to the chat to keep him in the loop). But ideally we'd just check if the output of ivy.function_[un]supported_[devices_and]_dtypes is a tuple. Then get a union of the unsupported dtypes of the 2. Finally after generating the data (function arguments) with those dtypes, we'd know whether we'd be deferring to the compositional implementation or the primary implementation. So based on the implementation we'd use, we'll find if the dtype generated is unsupported and just skip the test if it is unsupported for that particular dtype. Of course, this would mean we waste some examples, but I'm not sure if there's another way to test partial mixed functions without a larger redesign of the testing pipeline. And given that there's only a few partial mixed functions in the framework, I'm not sure if it's worth redesigning the testing pipeline altogether to incorporate these functions. Happy to know what you guys think, thanks slightly_smiling_face

Another option is to write the tests for the compositional and primary implementations separately. In this case, we would need to add an argument to the handle_test decorator say mixed_function which can take two values compositional or primary depending on which implementation we are testing. And then we would consider only one of the tuples returned by the ivy.function_supported_devices_and_datatypes based on the value of this argument. In this case, we would need to update the tests of all the partial mixed functions we have implemented ( i guess this is not more than 3-4). Again, not sure if this is the best solution here, would love to hear your thoughts @VedPatwardhan @CatB1t

[CatB1t]

1. could make the implementation a bit cleaner, because the intersection of unsupported dtypes for the backend-specific implementation and the compositional implementation wouldn't necessarily be the correct set of unsupported dtypes.

Thanks for the detailed review @VedPatwardhan rocket Completely agree with cleaning up the implementation part - will add a private helper function that all of these functions can use. I also agree that returning a tuple containing the tuple of unsupported dtypes for the backend-specific implementation and the tuple of unsupported dtypes for the compositional implementation makes much more sense, thanks for pointing this out +1 . In this case though, how should I update the handle_test decorator which expects that this function returns only one tuple containing the datatypes that are supported by the function to be tested ?

That's a great point! I think it would be a bit tricky in the tests (I'm also pinging @CatB1t to the chat to keep him in the loop). But ideally we'd just check if the output of ivy.function_[un]supported_[devices_and]_dtypes is a tuple. Then get a union of the unsupported dtypes of the 2. Finally after generating the data (function arguments) with those dtypes, we'd know whether we'd be deferring to the compositional implementation or the primary implementation. So based on the implementation we'd use, we'll find if the dtype generated is unsupported and just skip the test if it is unsupported for that particular dtype. Of course, this would mean we waste some examples, but I'm not sure if there's another way to test partial mixed functions without a larger redesign of the testing pipeline. And given that there's only a few partial mixed functions in the framework, I'm not sure if it's worth redesigning the testing pipeline altogether to incorporate these functions. Happy to know what you guys think, thanks slightly_smiling_face

Another option is to write the tests for the compositional and primary implementations separately. In this case, we would need to add an argument to the handle_test decorator say mixed_function which can take two values compositional or primary depending on which implementation we are testing. And then we would consider only one of the tuples returned by the ivy.function_supported_devices_and_datatypes based on the value of this argument. In this case, we would need to update the tests of all the partial mixed functions we have implemented ( i guess this is not more than 3-4). Again, not sure if this is the best solution here, would love to hear your thoughts @VedPatwardhan @CatB1t

That sounds like a good approach to me! I think trying to handle this dynamically would be painful and requires a lot of changes, the best solution as you mentioned is to write separate tests for primary implementations, but also a point to concern, would that approach work when there's a function there a mixed function 2 backends that have a primary implementation?

One naive approach that I can think of is to use write a test_mixed_function in function_testing which would call the primary implementation first, if it fails due then it will call the compositional implementation, the only drawback would be to actually get a good error message, as we're not sure if one failed due to unsupported dtype or an implementation error.

[hello-fri-end]

So I spent some time thinking about the possible solutions we discussed. Here's a summary:

1. Run the primary implementation first, if it fails due to dtype error, run the compositional implementation. Problem: challenging to detect dtype error e.g LayerNorm fails with the following error message: ivy.utils.exceptions.IvyBackendException: torch: layer_norm: "LayerNormKernelImpl" not implemented for 'Half' Not possible to implement.

2. Writing multiple strategies Problem: If multiple backends have mixed partial implementations then we will have to write a large number of strategies to cover the entire testing space because the conditions for switching between the implementations for different backends could have some overlap. Not possible to implement.

3. Generate the data, check which implementation will be used, and discard the example if the function doesn't support the data type of the example. Problem: Unbalanced testing examples i.e it's possible that, for example, we could end up discarding all the examples hypothesis generates for the compositional implementation because the data type is not supported. Plus, this approach will be very slow. Possible to implement.

4. Write the tests for backends that have mixed partial implementations separately. And write separate tests for primary and compositional implementations. Problem: We will end up wasting the examples we generate for the backends that are not being tested in each test. Possible to implement.

So, it looks like we have to choose between approach (3) or (4). I would personally prefer the 4th approach because it will cover the entire testing space and require the least amount of refactoring of testing helpers. Would love to know your thoughts @VedPatwardhan @CatB1t and then I'll start implementing one of these.

[CatB1t]

Tbh I'm kinda lost from the last comment 😂, let's have a discord call to discuss this more further @VedPatwardhan @hello-fri-end

[hello-fri-end]

Hi guys @VedPatwardhan @CatB1t! Here's a summary of changes I have made till now:

• Added private helper functions in ivy/data, ivy/general, ivy/device to handle <devices/dtypes> for mixed functions correctly. For mixed partial functions, the relevant dtypes/devices will be returned for both the compositional implementation as well as the primary implementation as a tuple.
• Fixed the recurse option in function_<supported/unsupported>_devices_and_dtypes by adding a recurse option in _get_devices_and_dtypes which by default is False so that function.supported_devices & function_supported_dtypes do not recurse into the input fn and recursion can be handled by the function_<supported/unsupported>_devices_and_dtypes itself.
• Updated _get_supported_devices_dtypes to correctly handle the tuple of dictionaries returned by ivy.function_supported_devices_and_dtypes in case of mixed functions.

For mixed partial functions, I've made the following changes:

• I added a parameter called mixed_function_index to the get_dtypes helper function which indicates whether to select the dtypes of the compositional implementation or the primary implementation. If it's 0, we select the types of the compositional implementation and if it's 1 we return the dtypes of the primary implementation.
• I updated all the functions that internally call get_dtypes to also have this parameter.

The idea behind this is that we can use hypothesis to generate this parameter for us and then based on the value of this parameter and the current backend, we generate the example for testing the function. e.g I have updated the test of ivy.linear in the last commit: https://github.com/unifyai/ivy/blob/98634861082dbfe61914b6bd77dbe7e42803d269/ivy_tests/test_ivy/test_functional/test_nn/test_layers.py#L19-L23 and once we have the correct set of dtypes, we can generate the data based on the backend and the value of this parameter: https://github.com/unifyai/ivy/blob/98634861082dbfe61914b6bd77dbe7e42803d269/ivy_tests/test_ivy/test_functional/test_nn/test_layers.py#L36-L37

Wanted to know your thoughts on this. If this is okay, I'll start updating the tests of all the other mixed functions and write a guide in the docs explaining all this.

[hello-fri-end]

Hey @hello-fri-end, I just wanted to ask if you've explored a way to call the same function twice recursively if it's a mixed function (once for the compositional one and once for the primary one) rather than adding additional helpers which seem to kinda perform the same logic? Also can we also return a dict instead of a tuple, would be useful for people to easily know which one's compositional and which one's primary I mean if we can just something like

def function_unsupported_dtypes(fn, ...):
    if it's a partial mixed function:
        return {
            'compositional': function_unsupported_dtypes(compos), 
            'primary': function_unsupported_dtypes(primary)
        }
    else:
        The usual logic

it would be really useful to reduce duplication. Would be happy to learn more about the challenges faced while implementing this (as I think you may have thought of this already), but that's a cleaner approach in my opinion. The changes made to the testing pipeline look good to me, of course you'll have to change the index logic to a boolean as @CatB1t suggested yesterday. Thanks slightly_smiling_face

Hey @VedPatwardhan, initially I tried almost the exact same thing that you have described above. The problem is with this approach is that it gets stuck into an infinite recursion loop because when the function is recursively called with the primary function as the input ( function_unsupported_dtypes(primary)) rhe condition for checking if the input function is a partial mixed function again evaluates to True. But thinking about it again, I don't think there's any need to recurse for the primary function as the _get_dtypes() helper is sufficient to get the supported/unsupported datatypes, so I think the following approach should work:

 def function_unsupported_dtypes(fn, ...):
     if it's a partial mixed function:
         return {
             'compositional': function_unsupported_dtypes(compos), 
             'primary': `_get_dtypes(primary)`
         }
     else:
         The usual logic

Will experiment with this and update the functions tomorrow. I also wanted to ask what the ideal behavior of this function should in the following case:

Assume that _fn is a mixed partial function:

fn_module = 'ivy.functional`
_tmp_mod = importlib.import_module(fn_module)
_fn = _tmp_mod.__dict__[fn_name]
ivy.function_supported_dtypes(_fn)

In this case, we are calling function_supported_dtypes(_fn) with the compositional function as the input but the backend has a mixed partial implementation. Should we just return the supported dtypes for the compositional implementation in this case @VedPatwardhan ?

[hello-fri-end]

Hey @hello-fri-end, I just wanted to ask if you've explored a way to call the same function twice recursively if it's a mixed function (once for the compositional one and once for the primary one) rather than adding additional helpers which seem to kinda perform the same logic? Also can we also return a dict instead of a tuple, would be useful for people to easily know which one's compositional and which one's primary I mean if we can just something like

def function_unsupported_dtypes(fn, ...):
    if it's a partial mixed function:
        return {
            'compositional': function_unsupported_dtypes(compos), 
            'primary': function_unsupported_dtypes(primary)
        }
    else:
        The usual logic

it would be really useful to reduce duplication. Would be happy to learn more about the challenges faced while implementing this (as I think you may have thought of this already), but that's a cleaner approach in my opinion. The changes made to the testing pipeline look good to me, of course you'll have to change the index logic to a boolean as @CatB1t suggested yesterday. Thanks slightly_smiling_face

Hey @VedPatwardhan, initially I tried almost the exact same thing that you have described above. The problem is with this approach is that it gets stuck into an infinite recursion loop because when the function is recursively called with the primary function as the input ( function_unsupported_dtypes(primary)) rhe condition for checking if the input function is a partial mixed function again evaluates to True. But thinking about it again, I don't think there's any need to recurse for the primary function as the _get_dtypes() helper is sufficient to get the supported/unsupported datatypes, so I think the following approach should work:

 def function_unsupported_dtypes(fn, ...):
     if it's a partial mixed function:
         return {
             'compositional': function_unsupported_dtypes(compos), 
             'primary': `_get_dtypes(primary)`
         }
     else:
         The usual logic

Will experiment with this and update the functions tomorrow. I also wanted to ask what the ideal behavior of this function should in the following case:

Hey @VedPatwardhan, so i tested this and it seems to work as expected. Will update all the functions with this logic,

Assume that _fn is a mixed partial function:

fn_module = 'ivy.functional.ivy`
_tmp_mod = importlib.import_module(fn_module)
_fn = _tmp_mod.__dict__[fn_name]
ivy.function_supported_dtypes(_fn)

In this case, we are calling function_supported_dtypes(_fn) with the compositional function as the input but the backend has a mixed partial implementation. Should we just return the supported dtypes for the compositional implementation in this case @VedPatwardhan ?

What should we do about this?

[hello-fri-end]

Apologies for the delay! I couldn't fully follow, wouldn't we have both those implementations in the same input function, given that we were storing it in the compos attribute? Ideally I think we should be computing the supported/unsupported for both the implementations when it's a partial mixed function. Thanks for looking into this @hello-fri-end slightly_smiling_face

Yes, we do have access to both implementations with the primary function, that's not what I meant. Actually, the way the handle_test decorator passes the input to function.supported_devices_and_dtypes is using the way I described above i.e by importing ivy.functional module and then retrieving the function through the dict which always returns the compositional implementation. For now, I have changed it to always pass ivy.__dict__[fn.__name__] incase of mixed functions, which will automatically pass the compositional implementation or the primary implementation based on the backend. And in case the backend has a mixed partial implementation and we pass the compositional implementation ( like in the decorator), we'll only return the supported/unsupporterted dtypes for the compositional implementation. Hope that makes sense. Please let me know if should update it to return both dtypes in that case too.

[vedpatwardhan]

Apologies for the delay! I couldn't fully follow, wouldn't we have both those implementations in the same input function, given that we were storing it in the compos attribute? Ideally I think we should be computing the supported/unsupported for both the implementations when it's a partial mixed function. Thanks for looking into this @hello-fri-end slightly_smiling_face

Yes, we do have access to both implementations with the primary function, that's not what I meant. Actually, the way the handle_test decorator passes the input to function.supported_devices_and_dtypes is using the way I described above i.e by importing ivy.functional module and then retrieving the function through the dict which always returns the compositional implementation. For now, I have changed it to always pass ivy.__dict__[fn.__name__] incase of mixed functions, which will automatically pass the compositional implementation or the primary implementation based on the backend. And in case the backend has a mixed partial implementation and we pass the compositional implementation ( like in the decorator), we'll only return the supported/unsupporterted dtypes for the compositional implementation. Hope that makes sense. Please let me know if should update it to return both dtypes in that case too.

But shouldn't the dict always return the primary implementation instead of the compositional one when it exists? Probably something to fix in the tests, any ideas @CatB1t? Thanks 🙂

[hello-fri-end]

Apologies for the delay! I couldn't fully follow, wouldn't we have both those implementations in the same input function, given that we were storing it in the compos attribute? Ideally I think we should be computing the supported/unsupported for both the implementations when it's a partial mixed function. Thanks for looking into this @hello-fri-end slightly_smiling_face

Yes, we do have access to both implementations with the primary function, that's not what I meant. Actually, the way the handle_test decorator passes the input to function.supported_devices_and_dtypes is using the way I described above i.e by importing ivy.functional module and then retrieving the function through the dict which always returns the compositional implementation. For now, I have changed it to always pass ivy.__dict__[fn.__name__] incase of mixed functions, which will automatically pass the compositional implementation or the primary implementation based on the backend. And in case the backend has a mixed partial implementation and we pass the compositional implementation ( like in the decorator), we'll only return the supported/unsupporterted dtypes for the compositional implementation. Hope that makes sense. Please let me know if should update it to return both dtypes in that case too.

But shouldn't the dict always return the primary implementation instead of the compositional one when it exists? Probably something to fix in the tests, any ideas @CatB1t? Thanks slightly_smiling_face

Here's a minimal example with torch backend for layer_norm

test = importlib.import_module('ivy.functional')
test.__dict__[fn_name].__module__
'ivy.functional.ivy.norms'

[vedpatwardhan]

Here's a minimal example with torch backend for layer_norm

test = importlib.import_module('ivy.functional')
test.__dict__[fn_name].__module__
'ivy.functional.ivy.norms'

Oh okay got it 👍 Can we hop on another call tomorrow about this? Thanks @CatB1t @hello-fri-end 🙂

[hello-fri-end]

Hey @VedPatwardhan @CatB1t ! This PR has been stagnant for a long time now because we decided to wait for the tests of interpolate to be fixed on master. They are still failing and it may take a while till they're fixed completely. I can update them in a separate PR once @sherry30 is done fixing them. Meanwhile, I think we should start the review process on this one. Here's a high-level summary of the changes I have made till now:

• Updated all the relevant devices and datatypes functions to return the unsupported/supported devices/dtypes for mixed functions correctly. In the case of partial mixed functions, a dictionary containing the dtypes/devices for the primary and compositional implementations will be returned. And for normal mixed functions, a tuple containing the dtypes/devices of either the primary implementation or the compositional implementation will be returned.

• Added a boolean parameter called mixed_fn_compos to get_dtypes ( and all the functions that make a call to get_dtypes). When it's True, the supported dtypes for the compositional implementation are returned and when False, it returns the supported dtypes for the primary implementation.

• Except for interpolate, the tests for all the mixed partial functions have been updated to first generate a value for this parameter using hypothesis and then based on the value of this parameter and the current backend, generate the appropriate data to test the function.

Looking forward to hearing your thoughts on these changes @VedPatwardhan @CatB1t :smiley:

[ivy-leaves]

If you are working on an open task, please edit the PR description to link to the issue you've created.

For more information, please check ToDo List Issues Guide.

Thank you :hugs:

[hello-fri-end]

Hey Ved, I added a fix for Trello: Edge cases of unsupported dtypes for compositional ivy functions in this commit https://github.com/unifyai/ivy/pull/14082/commits/e77f30d0a013e7159fc30eed3da0f9de37e083c7 here because the recurse option doesn't work correctly on master. Basically, to flag unsupported dtypes in the compositional implementation, you can simply add the unsupported_dtypes attribute to the function with a tuple containing the data-types and these will be added to the set of unsupported dtypes for that function. Happy to know your thoughts on this fix.

[hello-fri-end]

Hey Ved, I added a fix for Trello: Edge cases of unsupported dtypes for compositional ivy functions in this commit e77f30d here because the recurse option doesn't work correctly on master. Basically, to flag unsupported dtypes in the compositional implementation, you can simply add the unsupported_dtypes attribute to the function with a tuple containing the data-types and these will be added to the set of unsupported dtypes for that function. Happy to know your thoughts on this fix.

Hey @hello-fri-end, thanks for looking into this! Not sure why we'd need to define the unsupported dtypes for this compositional implementation though. Given that we're using ivy.frombuffer which has the with_unsupported_dtypes decorator with a tensorflow backend, shouldn't the unsupported_dtype function infer the dtype for as_strided? I think the only exception to this should be the einops functions which don't have backend implementations and make use of the einops API which don't have any unsupported dtype specified sweat_smile Happy to know your thoughts slightly_smiling_face

Actually, the problem in the compositional implementation of ivy.as_strided is that it uses the python memoryview function which doesn't support bfloat16 so there's no way of inheriting this from the primary functions called and we need a way of explicitly specifying it in the compositional implementation (first bullet point in the Trello card)

[vedpatwardhan]

Hey Ved, I added a fix for Trello: Edge cases of unsupported dtypes for compositional ivy functions in this commit e77f30d here because the recurse option doesn't work correctly on master. Basically, to flag unsupported dtypes in the compositional implementation, you can simply add the unsupported_dtypes attribute to the function with a tuple containing the data-types and these will be added to the set of unsupported dtypes for that function. Happy to know your thoughts on this fix.

Hey @hello-fri-end, thanks for looking into this! Not sure why we'd need to define the unsupported dtypes for this compositional implementation though. Given that we're using ivy.frombuffer which has the with_unsupported_dtypes decorator with a tensorflow backend, shouldn't the unsupported_dtype function infer the dtype for as_strided? I think the only exception to this should be the einops functions which don't have backend implementations and make use of the einops API which don't have any unsupported dtype specified sweat_smile Happy to know your thoughts slightly_smiling_face

Actually, the problem in the compositional implementation of ivy.as_strided is that it uses the python memoryview function which doesn't support bfloat16 so there's no way of inheriting this from the primary functions called and we need a way of explicitly specifying it in the compositional implementation (first bullet point in the Trello card)

Got it! Seems like I misunderstood the task, thanks for elaborating 😅

[vedpatwardhan]

In that case, we could actually generalize the check we're making for einops functions explicitly, cuz both functions (einops_reduce and as_strided) have the same attribute being used, happy to know what do you think @hello-fri-end 🙂

[hello-fri-end]

einops_reduce

Yeah, I think that makes sense. We can just have the has_unsupported_dtypes_attr = hasattr(fn, "unsupported_dtypes") check for both.

[hello-fri-end]

ivy-gardener :white_check_mark: Ivy gardener has formatted your code. If changes are requested, don't forget to pull your fork.

[hello-fri-end]

The errors on the CI seem to be unrelated to this PR. Merging this now - thanks for the reviews @vedpatwardhan @CatB1t :rocket: