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

Transition to arraycontext #56

Draft
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

alexfikl
Copy link
Contributor

@alexfikl alexfikl commented Jun 24, 2022

Since this is a big breaking change, took the opportunity to remove some things that were marked as deprecated:

  • FMMTraversalInfo.colleagues_starts and FMMTraversalInfo.colleagues_lists
  • filter_target_lists_in_user_order and filter_target_lists_in_tree_order
  • Remove DeviceDataRecord.

TODO:

@alexfikl
Copy link
Contributor Author

alexfikl commented Jun 24, 2022

As discussed, it seems to make sense to transition from DeviceDataRecord to arraycontext.dataclass_array_container. The operations needed are the same

  • DeviceDataRecord.get to arraycontext.to_numpy.
  • DeviceDataRecord.to_device to arraycontext.from_numpy.
  • DeviceDataRecord.with_queue to actx.freeze or actx.thaw (as necessary).
  • DeviceDataRecord.to_host_device_array to ??. Not sure about this one, but it seems like it should be deprecated and removed?

None of the classes based on DeviceDataRecord require any arithmetic, just to be serialized and deserialized.

@inducer
Copy link
Owner

inducer commented Jun 24, 2022

  • DeviceDataRecord.to_host_device_array to ??. Not sure about this one, but it seems like it should be deprecated and removed?

Yes, I think that's the right approach. The need for this arises mostly within the distributed-memory FMM, which @gaohao95 is working on. (@gaohao95, it would be useful if you could comment!) The point is that this needs tree information on the host to inform the realization of MPI communication, and it needs it on the device to support use of tree data in the translation operators. I think it will be cleaner to have two trees, one host-side, and one device-side, in place of the dual-storage solution at the array level that is currently in place.

An alternative would be to have an array context for these host-device arrays, but that seems perhaps more heavyweight than I'd like. A complicating factor is that we do not currently have a numpy array context (cf. inducer/arraycontext#93).

@gaohao95
Copy link
Contributor

gaohao95 commented Jun 26, 2022

I think it will be cleaner to have two trees, one host-side, and one device-side, in place of the dual-storage solution at the array level that is currently in place.

For global trees, we are already maintaining two copies of trees, one in the device memory and another in the host memory. I think the only place that uses the ImmutableHostDeviceArray at the moment is the local tree returned by generate_local_tree, but it is only used for passing into a traversal builder, so for that we could just return the device array version.

An alternative would be to have an array context for these host-device arrays, but that seems perhaps more heavyweight than I'd like.

I agree we shouldn't do this. ImmutableHostDeviceArray was only a stopgap solution in the past. We should remove all to_host_device_array and ImmutableHostDeviceArray.

@alexfikl
Copy link
Contributor Author

@inducer @gaohao95 Thanks for the feedback on that! I'll try removing ImmutableHostDeviceArray support and bug you once this gets into a better state.

@alexfikl alexfikl force-pushed the towards-array-context branch 5 times, most recently from d9f7872 to 8090d4b Compare June 28, 2022 12:42
Comment on lines -4 to -9
# Needed for boxtree.tools
- arg: init-hook
val: import sys; sys.setrecursionlimit(2000)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what in boxtree.tools was overworking pylint, but it doesn't seem necessary anymore (?).

@alexfikl alexfikl force-pushed the towards-array-context branch 6 times, most recently from ab5cde3 to c9ba4ca Compare June 29, 2022 11:07
@alexfikl alexfikl force-pushed the towards-array-context branch from c9ba4ca to 627990f Compare July 5, 2022 13:50
@alexfikl alexfikl force-pushed the towards-array-context branch from 627990f to 554a18f Compare August 2, 2022 06:30
@alexfikl alexfikl force-pushed the towards-array-context branch 2 times, most recently from 0b954e3 to 7711246 Compare August 24, 2022 11:24
@alexfikl alexfikl force-pushed the towards-array-context branch from 7711246 to 591e605 Compare August 24, 2022 12:07
@alexfikl alexfikl force-pushed the towards-array-context branch 2 times, most recently from e6ce7b9 to fd08060 Compare September 1, 2022 10:50
@alexfikl alexfikl force-pushed the towards-array-context branch 3 times, most recently from b254bc4 to 939f140 Compare September 10, 2022 10:31
@alexfikl alexfikl force-pushed the towards-array-context branch 2 times, most recently from e20af17 to 4978392 Compare September 18, 2022 06:35
@alexfikl alexfikl force-pushed the towards-array-context branch 2 times, most recently from 9f52cce to 9919b0e Compare November 15, 2024 09:55
@alexfikl alexfikl force-pushed the towards-array-context branch from 9919b0e to 9e7f9b2 Compare November 15, 2024 14:23
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