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

[Tests] Fix tests 3rd ed. #215

Merged
merged 15 commits into from
Sep 18, 2024
Merged

[Tests] Fix tests 3rd ed. #215

merged 15 commits into from
Sep 18, 2024

Conversation

zhzhcookie
Copy link
Collaborator

@zhzhcookie zhzhcookie commented Sep 14, 2024

PR Category

OP Test

Type of Change

Tests

Description

  1. Change --device=cpu to --ref=cpu
  2. Add --mode=quick to test only one shape for CI cpu tests.
  3. Add scalar shape for pointwise_dynamic op tests
  4. Move assert_close and assert_equal to flag_gems.testing
  5. Add mark for each op test, and register it to pytest.ini
  6. Add speedup in benchmark print result.

Issue

Progress

  • Change is properly reviewed (1 reviewer required, 2 recommended).
  • Change is responded to an issue.
  • Change is fully covered by a UT.

Performance

Comment on lines 13 to 18
def assert_close(res, ref, dtype, equal_nan=False, reduce_dim=1):
ref = ref.to(dtype)
atol = 1e-4 * reduce_dim
rtol = RESOLUTION[dtype]
torch.testing.assert_close(res, ref, atol=atol, rtol=rtol, equal_nan=equal_nan)
Copy link
Contributor

Choose a reason for hiding this comment

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

Two issues:

  • fp64 is not in RESOLUTION's key set.
  • passing a dtype matching neither ref nor res may raise an error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, do we consider default dtype=None? Now dtype is mandate.

Copy link
Collaborator Author

@zhzhcookie zhzhcookie Sep 14, 2024

Choose a reason for hiding this comment

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

Two issues:

  • fp64 is not in RESOLUTION's key set.
  • passing a dtype matching neither ref nor res may raise an error.

Now only isclose & allclose test fp64. These two tests do not use assert_close.
Maybe we can assert res.dtype == dtype here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Two issues:

  • fp64 is not in RESOLUTION's key set.
  • passing a dtype matching neither ref nor res may raise an error.

Now only isclose & allclose test fp64. These two tests do not use assert_close. Maybe we can assert res.dtype == dtype here?

done

@@ -21,6 +21,11 @@
from .conftest import TO_CPU


def remove_zero(inp):
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rename this as replace_zeros

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would rename this as replace_zeros

done

Comment on lines 78 to 79
if upcast:
ref_inp = ref_inp.to(torch.float64)
Copy link
Contributor

Choose a reason for hiding this comment

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

what about integer? we are not supposed to upcast integers to floats in all occasions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

what about integer? we are not supposed to upcast integers to floats in all occasions.

Now to_reference is only for float inputs.

if TO_CPU:
a = a.to("cpu")
assert torch.equal(a, b)
def gems_assert_close(res, ref, dtype, equal_nan=False, reduce_dim=1):
Copy link
Contributor

Choose a reason for hiding this comment

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

what's different between this and testing.assert_close?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

what's different between this and testing.assert_close?

TO_CPU here.

CUDA_VISIBLE_DEVICES=7 coverage run ${COVERAGE_ARGS} -m pytest -s tests/test_special_ops.py --ref=cpu --mode=quick && \
CUDA_VISIBLE_DEVICES=7 coverage run ${COVERAGE_ARGS} -m pytest -s tests/test_pointwise_type_promotion.py --ref=cpu --mode=quick && \
CUDA_VISIBLE_DEVICES=7 coverage run ${COVERAGE_ARGS} -m pytest -s tests/test_tensor_constructor_ops.py --ref=cpu --mode=quick && \
bash tools/pytest_mark_check.sh &"
Copy link
Collaborator

@Bowen12992 Bowen12992 Sep 18, 2024

Choose a reason for hiding this comment

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

it's better to put pytest_mark_check.sh before running the unit tests. Or use an additional ci pipeline named pytest_mark_check to run this check

CUDA_VISIBLE_DEVICES=7 coverage run ${COVERAGE_ARGS} -m pytest -s tests/test_binary_pointwise_ops.py --ref=cpu --mode=quick && \
CUDA_VISIBLE_DEVICES=7 coverage run ${COVERAGE_ARGS} -m pytest -s tests/test_special_ops.py --ref=cpu --mode=quick && \
CUDA_VISIBLE_DEVICES=7 coverage run ${COVERAGE_ARGS} -m pytest -s tests/test_pointwise_type_promotion.py --ref=cpu --mode=quick && \
CUDA_VISIBLE_DEVICES=7 coverage run ${COVERAGE_ARGS} -m pytest -s tests/test_tensor_constructor_ops.py --ref=cpu --mode=quick && \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
CUDA_VISIBLE_DEVICES=7 coverage run ${COVERAGE_ARGS} -m pytest -s tests/test_tensor_constructor_ops.py --ref=cpu --mode=quick && \
CUDA_VISIBLE_DEVICES=7 coverage run ${COVERAGE_ARGS} -m pytest -s tests/test_tensor_constructor_ops.py --ref=cpu --mode=quick &

Copy link
Collaborator

@Bowen12992 Bowen12992 left a comment

Choose a reason for hiding this comment

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

CUDA_VISIBLE_DEVICES=7 coverage run ${COVERAGE_ARGS} -m pytest -s tests/test_special_ops.py --ref=cpu --mode=quick && \
CUDA_VISIBLE_DEVICES=7 coverage run ${COVERAGE_ARGS} -m pytest -s tests/test_pointwise_type_promotion.py --ref=cpu --mode=quick && \
CUDA_VISIBLE_DEVICES=7 coverage run ${COVERAGE_ARGS} -m pytest -s tests/test_tensor_constructor_ops.py --ref=cpu --mode=quick &"
"bash tools/pytest_mark_check.sh &"
Copy link
Collaborator

Choose a reason for hiding this comment

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

[pytest]
filterwarnings =
    ignore::pytest.PytestUnknownMarkWarning

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

[pytest]
filterwarnings =
    ignore::pytest.PytestUnknownMarkWarning

done

Copy link
Collaborator

@Bowen12992 Bowen12992 left a comment

Choose a reason for hiding this comment

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

LGTM

@zhzhcookie zhzhcookie merged commit 8536ebb into master Sep 18, 2024
4 checks passed
@zhzhcookie zhzhcookie deleted the fix_tests_3rd_202409 branch September 18, 2024 03:05
DuanYaQi pushed a commit that referenced this pull request Sep 20, 2024
1. Change --device=cpu to --ref=cpu
2. Add --mode=quick to test only one shape for CI cpu tests.
3. Add scalar shape for pointwise_dynamic op tests
4. Move assert_close and assert_equal to flag_gems.testing
5. Add mark for each op test, and register it to pytest.ini
6. Add speedup in benchmark print result.

---------

Co-authored-by: zhengyang <[email protected]>
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.

3 participants