Skip to content

Use arraycontext instead of pyopencl - #128

Merged
inducer merged 1 commit into
inducer:mainfrom
alexfikl:no-pycl
Feb 18, 2022
Merged

Use arraycontext instead of pyopencl#128
inducer merged 1 commit into
inducer:mainfrom
alexfikl:no-pycl

Conversation

@alexfikl

@alexfikl alexfikl commented Jan 15, 2022

Copy link
Copy Markdown
Collaborator

Inspired by inducer/arraycontext#111, this tried to remove some of the direct usage of pyopencl. It's still a far shot from actually working with TaggedCLArray transparently, but it's a start! This mostly removes uses of

  • cl.array parts which are in arraycontext
  • ary.with_queue(queue) with actx.thaw or actx.freeze
  • ary.get() with actx.from_numpy

A few obvious places where pyopencl is still used:

  • some calls to cl.wait_for_events; not sure how necessary this is?
  • some calls to pyopencl.algoritm: copy_if
  • some calls to pyopencl.array: take.
  • some kernels: ElementwiseKernel, GenericScanKernel

The main roadblock however seems to be that all the sumpy calls return bare cl.array.Array and generally don't know anything about array contexts.

@alexfikl
alexfikl force-pushed the no-pycl branch 2 times, most recently from 9706e35 to 2271297 Compare January 15, 2022 21:16
@inducer

inducer commented Jan 16, 2022

Copy link
Copy Markdown
Owner

It's still a far shot from actually working, but it's a start!

What do you mean? CI seems to be passing...

@alexfikl

alexfikl commented Jan 16, 2022

Copy link
Copy Markdown
Collaborator Author

It's still a far shot from actually working, but it's a start!

What do you mean? CI seems to be passing...

Ah, I meant from working seamlessly with that TaggedCLArray thing from inducer/arraycontext#111. Well, the version of the PR that only had self.array_types = (TaggedCLArray,) (might have made this PR before I noticed @kaushikcfd fixed things in there).

@inducer

inducer commented Jan 16, 2022

Copy link
Copy Markdown
Owner

take

It should be possible to replace at least that with fancy indexing.

@alexfikl

Copy link
Copy Markdown
Collaborator Author

It should be possible to replace at least that with fancy indexing.

I actually went the other way and left everything in qbx.cost and qbx.fmm (where take was used) alone for now. That's talking to boxtree and sumpy that only know about queues, so it's very awkward to insert an array context in there at the moment.

@alexfikl
alexfikl requested a review from inducer January 18, 2022 20:12
@alexfikl
alexfikl marked this pull request as draft January 19, 2022 03:27
@alexfikl
alexfikl force-pushed the no-pycl branch 2 times, most recently from b47246a to b0a13f1 Compare January 19, 2022 17:48
Comment thread pytential/qbx/geometry.py Outdated
lists=targets_sorted_by_center).with_queue(None)

return result
actx.queue.finish()

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This seems to be introducing some synchronization issues. Most of the functions in QBXFMMGeometryData were creating their own queues to precompute data and this PR switched them to using the array context from the constructor and freezing the results.

Not sure why that would cause issues (it doesn't seem like anything is returned thawed), but it does 😖

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

After some playing around with this: it was only causing failures when using fmmlib where it was using a

with cl.CommandQueue() as queue:
	geo_data = ToHostTransferredGeoDataWrapper(queue, geo_data)

Switching that to using the array context inside geo_data seems to fix any failures. It still seems like something is a bit brittle though :(

@inducer

inducer commented Feb 18, 2022

Copy link
Copy Markdown
Owner

This was kind of painful to review, but I finally made it through. LGTM, thanks for working on it!

@alexfikl

Copy link
Copy Markdown
Collaborator Author

This was kind of painful to review, but I finally made it through. LGTM, thanks for working on it!

Sorry about that! Hopefully it will save some future pain with the array contexts :(

@inducer
inducer merged commit 51e4dc3 into inducer:main Feb 18, 2022
@alexfikl
alexfikl deleted the no-pycl branch February 18, 2022 02:14
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.

2 participants