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

FIX: prevent support_usm_ndarray from changing queue if explicitly provided. #1940

Merged
merged 3 commits into from
Jul 17, 2024
Merged

FIX: prevent support_usm_ndarray from changing queue if explicitly provided. #1940

merged 3 commits into from
Jul 17, 2024

Conversation

olegkkruglov
Copy link
Contributor

@olegkkruglov olegkkruglov commented Jul 16, 2024 •
edited
Loading

Description

  • Changed logic of support_usm_ndarray. Now it does not change the queue if it was explicitly provided to args. Previous logic does not allow to use np.array + SyclQueue interface with decorated method.
  • queue_param is removed from support_usm_ndarray since the check can be done via kwargs checking.
  • Docstring is added to support_usm_ndarray with highlighting the change
  • 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
  • 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.

@olegkkruglov olegkkruglov added the bug Something isn't working label Jul 16, 2024
@olegkkruglov
Copy link
Contributor Author

/intelci: run

samir-nasibli
samir-nasibli reviewed Jul 16, 2024
View reviewed changes
Copy link
Contributor

@samir-nasibli samir-nasibli left a comment

Choose a reason for hiding this comment

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

Thank you for founding out this issue.
This is critical for sklearnex.spmd ifaces with numpy + sycl_queue. Unfortunately we don't have properly testing yet for this API. Only some examples with dpnp/dpctl input, that not the case of this bug. Hopefully development on #1777 and #1909 will cover this cases as well.

I would like to have some tests in case. I can follow this add it here.

samir-nasibli
samir-nasibli reviewed Jul 16, 2024
View reviewed changes
@olegkkruglov olegkkruglov mentioned this pull request Jul 16, 2024
Merged
8 tasks
samir-nasibli
samir-nasibli reviewed Jul 16, 2024
View reviewed changes
Copy link
Contributor

@samir-nasibli samir-nasibli left a comment

Choose a reason for hiding this comment

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

Generally looks good to me. @ethanglaser please take a look as well. This is critical for spmd ifaces with numpy + sycl queue inputs.

@olegkkruglov olegkkruglov force-pushed the support_usm_ndarray-fix branch 3 times, most recently from 1b35d8b to dbe7e69 Compare July 16, 2024 14:11
samir-nasibli
samir-nasibli requested changes Jul 16, 2024
View reviewed changes
samir-nasibli
samir-nasibli reviewed Jul 16, 2024
View reviewed changes
Copy link
Contributor

@samir-nasibli samir-nasibli left a comment

Choose a reason for hiding this comment

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

We went a little bit wrong in our thoughts, but it seems like it should be normal now.

Again without testing this primitive we could possible have some other issues. It would be great to add tests to check for this estimator expected behavior for the dummy function. Could be done in separate PR as follow up, but make sure that we have ticket covered. Thank you!

@olegkkruglov olegkkruglov force-pushed the support_usm_ndarray-fix branch from a676415 to 6252d93 Compare July 16, 2024 14:40
samir-nasibli
samir-nasibli approved these changes Jul 16, 2024
View reviewed changes
Copy link
Contributor

@samir-nasibli samir-nasibli left a comment

Choose a reason for hiding this comment

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

LGTM. Assuming green CI

@olegkkruglov olegkkruglov force-pushed the support_usm_ndarray-fix branch from 6252d93 to 8403a00 Compare July 16, 2024 14:44
@ethanglaser
Copy link
Contributor

Please run black formatting on the file

@olegkkruglov olegkkruglov force-pushed the support_usm_ndarray-fix branch from 8403a00 to c4aacc7 Compare July 16, 2024 15:39
@olegkkruglov
Copy link
Contributor Author

/intelci: run

@samir-nasibli samir-nasibli merged commit 69d147f into uxlfoundation:main Jul 17, 2024
18 checks passed
@samir-nasibli
Copy link
Contributor

@mergify backport rls/2024.6.0-rls

@mergify Mergify
Copy link

mergify bot commented Jul 17, 2024 •
edited
Loading

backport rls/2024.6.0-rls

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Jul 17, 2024
@olegkkruglov @mergify
FIX: prevent support_usm_ndarray from changing queue if explicitly …
e83612d 
…provided. (#1940)

(cherry picked from commit 69d147f)

# Conflicts:
#	onedal/_device_offload.py
@mergify mergify bot mentioned this pull request Jul 17, 2024
Closed
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samir-nasibli samir-nasibli samir-nasibli approved these changes

@ethanglaser ethanglaser ethanglaser approved these changes

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

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

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@olegkkruglov @ethanglaser @samir-nasibli