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

[Good First Issue][NNCF]: Add tests for torch device utils #2579

Closed
daniil-lyakhov opened this issue Mar 15, 2024 · 5 comments
Closed

[Good First Issue][NNCF]: Add tests for torch device utils #2579

daniil-lyakhov opened this issue Mar 15, 2024 · 5 comments
Assignees
Labels
good first issue Good for newcomers

Comments

@daniil-lyakhov
Copy link
Collaborator

daniil-lyakhov commented Mar 15, 2024

Dear good first issue solvers, greetings!🐱
Don't miss the opportunity to contribute to our beloved project!

Context

Functions get_model_device, is_multidevice and get_model_dtype from the file https://github.com/openvinotoolkit/nncf/blob/develop/nncf/torch/utils.py are not covered by tests and don't have proper docstrings.

What needs to be done?

The tasks are
*) To extend file https://github.com/openvinotoolkit/nncf/blob/develop/tests/torch/test_utils.py with tests for functions mentioned above. Please check all 3 possible scenarios:

  1. Model has no parameters
  2. Model has parameters and all parameters placed on the same device
  3. Model has parameters and parameters placed on different devices

*) To add proper docstrings for functions mentioned above

Example Pull Requests

#2526

Resources

Contact points

@daniil-lyakhov

Ticket

No response

@daniil-lyakhov daniil-lyakhov added the good first issue Good for newcomers label Mar 15, 2024
@DaniAffCH
Copy link
Contributor

.take

Copy link

Thank you for looking into this issue! Please let us know if you have any questions or require any help.

@DaniAffCH
Copy link
Contributor

DaniAffCH commented Mar 15, 2024

Hi, @daniil-lyakhov thank you for having posted a GFI, I was looking for that :)

I was wondering whether it's safe to consider only the first network layer in the function get_model_dtype.

As far as I know, there are some edge cases in which torch allows you to use different dtype within the same models as long as you keep the two workflows separate.

Consider this example:

import torch
import torch.nn as nn

class MixedTypeNet(nn.Module):
    def __init__(self):
        super(MixedTypeNet, self).__init__()
        self.fc_f16 = nn.Linear(10, 1).to(torch.float16)
        self.fc_f32 = nn.Linear(10, 1).to(torch.float32)

    def forward(self, f16, f32):
        x_f16 = torch.relu(self.fc_f16(f16))
        x_f32 = torch.relu(self.fc_f32(f32))
        return x_f16, x_f32

device = torch.device("cuda")
net = MixedTypeNet().to(device)

input_data_f16 = torch.randn(1, 10, dtype=torch.float16).to(device)
input_data_f32 = torch.randn(1, 10, dtype=torch.float32).to(device)

x_f16, x_f32 = net(input_data_f16, input_data_f32)

print("Output (f16):", x_f16)
print("Output (f32):", x_f32)
print(next(net.parameters()).dtype)

In this case, the network contains 2 different types but next(net.parameters()).dtype would return torch.float16. Is this situation considered in the current get_model_dtype implementation? In case, is it relevant?

@daniil-lyakhov
Copy link
Collaborator Author

daniil-lyakhov commented Mar 18, 2024

Hi, @daniil-lyakhov thank you for having posted a GFI, I was looking for that :)

I was wondering whether it's safe to consider only the first network layer in the function get_model_dtype.

As far as I know, there are some edge cases in which torch allows you to use different dtype within the same models as long as you keep the two workflows separate.

Consider this example:

import torch
import torch.nn as nn

class MixedTypeNet(nn.Module):
    def __init__(self):
        super(MixedTypeNet, self).__init__()
        self.fc_f16 = nn.Linear(10, 1).to(torch.float16)
        self.fc_f32 = nn.Linear(10, 1).to(torch.float32)

    def forward(self, f16, f32):
        x_f16 = torch.relu(self.fc_f16(f16))
        x_f32 = torch.relu(self.fc_f32(f32))
        return x_f16, x_f32

device = torch.device("cuda")
net = MixedTypeNet().to(device)

input_data_f16 = torch.randn(1, 10, dtype=torch.float16).to(device)
input_data_f32 = torch.randn(1, 10, dtype=torch.float32).to(device)

x_f16, x_f32 = net(input_data_f16, input_data_f32)

print("Output (f16):", x_f16)
print("Output (f32):", x_f32)
print(next(net.parameters()).dtype)

In this case, the network contains 2 different types but next(net.parameters()).dtype would return torch.float16. Is this situation considered in the current get_model_dtype implementation? In case, is it relevant?

Hi @DaniAffCH, thank you for the collaboration! This is a nice catch! As function get_model_device returns only device of the fist params, I think it is valid for the get_model_dtype to return the dtype of the first parameter as well. As such cases like multi dtype model are not really common I suggest to mention "first parameter device/ dtype of the model if exist and default value elsewhere" and leave functions as is.

CC: @alexsu52

@github-project-automation github-project-automation bot moved this to Contributors Needed in Good first issues Mar 18, 2024
@alexsu52 alexsu52 moved this from Contributors Needed to Assigned in Good first issues Mar 18, 2024
AlexanderDokuchaev pushed a commit that referenced this issue Mar 18, 2024
### Changes

This PR addresses #2579.
- Add tests for torch device utils. The tests consider the case in which
the model has no parameters, has all parameters on CPU, has all
parameters on CUDA, and has parameters placed on different devices. In
the latter, the parameters are moved on different devices randomly.
- Add tests for torch `dtype` utils. The case in which the model has no
parameters is also considered.
- Created a test torch helper class `EmptyModel` to take into account
the case in which the model has no parameters at all.
- Add docstrings in utils.

### Tests

I compared all the results manually checking for their correctness. The
code is also compliant with the coding style having been verified with
`pre-commit run`

---------

Co-authored-by: Daniil Lyakhov <[email protected]>
@daniil-lyakhov
Copy link
Collaborator Author

@DaniAffCH, thank you for the contribution!

@github-project-automation github-project-automation bot moved this from Assigned to Closed in Good first issues Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
Archived in project
Development

No branches or pull requests

2 participants