Skip to content

Conversation

ahmadsharif1
Copy link
Contributor

@ahmadsharif1 ahmadsharif1 commented Oct 30, 2024

This addresses comments:

#319 (comment)
#319 (comment)

  1. Add a mark in conftest.py
  2. Add a decorator that passes in cpu and cuda devices
  3. Adds more tests for cuda, especially batch frame extraction tests
  4. Update the tensor_close function to be more tolerant of differences. The default is now 90% of the values should be within 20 of each other.

I was not able to reuse the logic in the needs_cuda() decorator so I just duplicated that logic. If someone has better ideas let me know and I can incorporate them.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Oct 30, 2024
@ahmadsharif1 ahmadsharif1 marked this pull request as ready for review October 30, 2024 17:12
test/utils.py Outdated

# Asserts that at least `percentage`% of the values are within the absolute tolerance.
def assert_tensor_close_on_at_least(frame1, frame2, percentage=99.7, abs_tolerance=20):
def assert_tensor_close_on_at_least(frame1, frame2, percentage=90, abs_tolerance=20):
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the default tolerance has changed, did that implicitly changed the tolerance of the existing calls as well? E.g. the ones introduced in https://github.com/pytorch/torchcodec/pull/319/files has a stricter tolerance IIUC - we may need to update those?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am fine using higher tolerances for those too -- as they may have different values for different GPU models anyway

Copy link
Contributor

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Thanks @ahmadsharif1

IIUC this PR mostly tests get_frames_at_indices, get_frames_displayed_at and get_frames_in_range.

But for coverage we probably want to also parametrize the existing tests for get_frame_at, get_frame_displayed_at, get_next_frame(), etc.

And we'll also want to add tests to the public decoder class.

I can address those in a follow-up if you'd like

def test_get_frames_at_indices(self):
@pytest.mark.parametrize("device", cpu_and_cuda())
def test_get_frames_at_indices(self, device):
tensor_compare_function = get_frame_compare_function(device)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: here and everywhere below, call this frame_compare_function

@ahmadsharif1
Copy link
Contributor Author

Thanks. Actually I just realized that in doing all the test refactors I missed a very important check -- we don't actually check that the frame is on the correct device.

@NicolasHug do you have any opinions on how it should be done.

I think we can even have a function like test_frame_against_reference(frame1, frame2, expected_device)

Within that function we can 1) assert frame is on device and 2) use the correct comparison function -- i.e. approximately equal for cuda and exactly equal for CPU

Thoughts?

As far as more tests are concerned, you (or I) can address those later. But let's align on the overall testing flow for cuda tests including testing the device and tensor values both.

@ahmadsharif1 ahmadsharif1 merged commit 76900f6 into meta-pytorch:main Nov 1, 2024
37 of 40 checks passed
@ahmadsharif1 ahmadsharif1 deleted the cuda15 branch November 1, 2024 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants