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.

Sign up for GitHub

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

Jump to bottom

MAINT: using common test primitive for banned primitives check in locations #1867

Merged
merged 7 commits into from
Oct 23, 2024
Merged

MAINT: using common test primitive for banned primitives check in locations #1867

merged 7 commits into from
Oct 23, 2024

Conversation

samir-nasibli
Copy link
Contributor

@samir-nasibli samir-nasibli commented Jun 14, 2024 •
edited
Loading

Description

Suggested changes:

  • Generalized for further use of checker for banned primitives

Discussed here #1839 (comment)

  • I have reviewed my changes thoroughly before submitting this pull request.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation to reflect the changes, if necessary (updated in # - add PR number)
  • The unit tests pass successfully.
  • I have run it locally and tested the changes extensively.
  • I have resolved any merge conflicts that might occur with the base branch.
  • Git commit message contains an appropriate signed-off-by string (see CONTRIBUTING.md for details)
  • I have added a respective label(s) to PR if I have a permission for that.

@samir-nasibli
Copy link
Contributor Author

/intelci: run

@samir-nasibli samir-nasibli marked this pull request as ready for review June 14, 2024 22:32
icfaust
icfaust reviewed Jun 15, 2024
View reviewed changes
Copy link
Contributor

@icfaust icfaust left a comment

Choose a reason for hiding this comment

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

I find the justification for the PR to be weak, but since the work has been done, I ask that you show that fuctionality hasn't been reduced. please show the CI failing for private CI and public CI for target_offload and sklearn_check_version when they are placed in the codebase. ideally by temporarily modifying your PR, running CIs and then returning them back.

@samir-nasibli
Copy link
Contributor Author

@icfaust Thank you for the feedback!
Can you please clarify what exactly was missing for justification, I will try to expand the PR description.
Yeah, it seem I should update this test runners.
Btw current internal CI fail is not related to the changes.

@icfaust
Copy link
Contributor

icfaust commented Jun 19, 2024

@icfaust Thank you for the feedback! Can you please clarify what exactly was missing for justification, I will try to expand the PR description. Yeah, it seem I should update this test runners. Btw current internal CI fail is not related to the changes.

The justification of this PR is that this is a movement since it is a developer-only feature. However, all tests are included in the install package, and those are for developers and not end users. That is also the case with sklearn, where we install sklearn and then directly use the testing suite as developers, even those it is in the user install.

I would like to see the tests throw a failure (to show they are working in CI). Could you add cases of sklearn_check_version and device_offload to show that the tests properly fail in CI? We can remove them later before merging.

@samir-nasibli
Copy link
Contributor Author

@icfaust Thank you for the feedback! Can you please clarify what exactly was missing for justification, I will try to expand the PR description. Yeah, it seem I should update this test runners. Btw current internal CI fail is not related to the changes.

The justification of this PR is that this is a movement since it is a developer-only feature. However, all tests are included in the install package, and those are for developers and not end users. That is also the case with sklearn, where we install sklearn and then directly use the testing suite as developers, even those it is in the user install.

I would like to see the tests throw a failure (to show they are working in CI). Could you add cases of sklearn_check_version and device_offload to show that the tests properly fail in CI? We can remove them later before merging.

Thank you @icfaust !

I thought about it once again and I think that your opinion is reseanoble and it really might be worth it to include this in standard testing of the package.
But I wouldn't want to turn off these tests. I would suggest muting cases if there are any disabled or exceptions.
In addition, I want to see how well these tests work, and whether they affect the validation process in our CI

@samir-nasibli samir-nasibli marked this pull request as draft June 20, 2024 10:27
@samir-nasibli samir-nasibli added the testing Tests for sklearnex/daal4py/onedal4py & patching sklearn label Jun 22, 2024
@samir-nasibli
reverting tests places for now
c5cc6e6 
just avoiding code duplication via _test_primitive_usage_ban use

merged main
@samir-nasibli
fix linting
4b5d85f
@samir-nasibli samir-nasibli changed the title TEST: moved checks for banned primitives into certain folder, out of … TEST: using common test primitive for banned primitives check in locations Oct 19, 2024
@samir-nasibli samir-nasibli requested a review from icfaust October 19, 2024 22:58
@samir-nasibli samir-nasibli marked this pull request as ready for review October 19, 2024 22:58
@samir-nasibli
Copy link
Contributor Author

@icfaust I have returned back tests, but generalized in a test primitive common parts to avoid code duplication.

@samir-nasibli samir-nasibli changed the title TEST: using common test primitive for banned primitives check in locations MAINT: using common test primitive for banned primitives check in locations Oct 19, 2024
icfaust
icfaust reviewed Oct 21, 2024
View reviewed changes
Copy link
Contributor

@icfaust icfaust left a comment

Choose a reason for hiding this comment

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

Big things are: spelling mistakes on 'primitive', using pkgutil instead of importlib (also in standard library), and a stylistic change from "_test" to "_check". Thanks also for catching some mistakes in the original implementation. These changes and we can merge quickly.

onedal/tests/test_common.py Outdated Show resolved Hide resolved
onedal/tests/test_common.py Show resolved Hide resolved
onedal/tests/test_common.py Outdated Show resolved Hide resolved
onedal/tests/test_common.py Outdated Show resolved Hide resolved
onedal/tests/test_common.py Outdated Show resolved Hide resolved
sklearnex/tests/test_common.py Outdated Show resolved Hide resolved
sklearnex/tests/test_common.py Outdated Show resolved Hide resolved
sklearnex/tests/test_common.py Show resolved Hide resolved
@samir-nasibli
refactoring
3a33d9b 
using pkgutil.get_loader instead of importlib.import_module
@samir-nasibli samir-nasibli requested a review from icfaust October 21, 2024 15:11
icfaust
icfaust reviewed Oct 23, 2024
View reviewed changes
Copy link
Contributor

@icfaust icfaust left a comment

Choose a reason for hiding this comment

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

Sorry again for making you change this again. This should now remove the added TODO.

onedal/tests/test_common.py Outdated Show resolved Hide resolved
onedal/tests/test_common.py Outdated Show resolved Hide resolved
@samir-nasibli
Copy link
Contributor Author

/intelci: run

@samir-nasibli
Copy link
Contributor Author

Sorry again for making you change this again. This should now remove the added TODO.

No problem. I tought we can update it later, but maybe make sense do it now. Applied suggestions. Thank you!

@samir-nasibli samir-nasibli requested a review from icfaust October 23, 2024 09:11
icfaust
icfaust approved these changes Oct 23, 2024
View reviewed changes
Copy link
Contributor

@icfaust icfaust left a comment

Choose a reason for hiding this comment

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

Let the CI run through. Merge ready assuming green

@samir-nasibli samir-nasibli merged commit 3936cb9 into uxlfoundation:main Oct 23, 2024
27 checks passed
@samir-nasibli samir-nasibli deleted the tests/maint_design_tests branch October 23, 2024 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@icfaust icfaust icfaust approved these changes

@ethanglaser ethanglaser Awaiting requested review from ethanglaser

@Alexsandruss Alexsandruss Awaiting requested review from Alexsandruss Alexsandruss is a code owner

Assignees
No one assigned
Labels
testing Tests for sklearnex/daal4py/onedal4py & patching sklearn
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@samir-nasibli @icfaust