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 #139

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

Conversation

alexfikl
Copy link
Contributor

@alexfikl alexfikl commented Sep 4, 2022

Get sumpy on array contexts as well! This eats up the first commit from #118.

Porting individual files

  • Pass tests!
    • test_codegen
    • test_cse
    • test_distributed
    • test_fmm
    • test_kernels
    • test_matrixgen
    • test_misc
    • test_qbx
    • test_tools
  • Port examples!
    • curve-pot
    • curve
    • expansion-toys
    • fourier
    • sym-exp-complexity

Remaining porting issues

  • Move caching to the array context from KernelCacheMixin.
  • Handle kernel optimizations.
    • Currently it uses get_cached_executor(), which branches on OPT_ENABLED, but we need to move to actx.call_loopy.
    • Move to transform_loopy_program? Would need to also add some fancy tags or something to customize it?
  • Need to move e.g. register_optimization_preambles to the array context somehow too.
    • The kernels no longer have a context or device, so they can't do that.
    • Probably need to move this to actx.transform_loopy_program somehow?

Upstream / downstream PRs

Speculative TODOs

  • Should some classes clone the actx? Here e.g. SumpyTreeIndependentDataForWrangler but also in boxtree.
    • The big worry is that cloning throws away any caches on the actx.
    • Pretty much all *Builder classes can be functions + all other memoized kernels can be functions, but should leave that for later since it can be done without breaking the world.
  • Rip out the timing stuff (not sure, just mentioned by @inducer at some point).
    • This would also need to be done in boxtree?
  • Add an array context to do the timing stuff? (likely for a future PR)

sumpy/e2p.py Outdated Show resolved Hide resolved
sumpy/e2p.py Outdated Show resolved Hide resolved
test/test_fmm.py Outdated
timing_data = {}
pot, = drive_fmm(actx, wrangler, (weights,), timing_data=timing_data)
logger.info("timing_data:\n%s", timing_data)

assert timing_data
pot, = drive_fmm(actx, wrangler, (weights,))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm. What's the recommended way to get the timing data?

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 quite sure. I briefly talked about that with @inducer and it should be baked into the array context somehow so it doesn't clutter the wrangler interface.

However, that probably needs a bit of effort to get back into a working state, so not quite sure what a timeline would be..

Copy link
Contributor Author

@alexfikl alexfikl Sep 25, 2022

Choose a reason for hiding this comment

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

Just to write out some questions about that to get an idea (from you and @inducer). I don't really use it so this has been a brutal delete sort of thing..

  • Do you rely on that interface to get results?
  • Can this be merged without it?
  • Who's going to do the work to put it back? 😢

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you rely on that interface to get results?

Yes, I use it to get results for a paper.

Who's going to do the work to put it back?

I can try if there's an outline of how to reintroduce it. I have not much experience on arraycontexts, so I have no idea what needs to be done.

Copy link
Contributor Author

@alexfikl alexfikl Sep 25, 2022

Choose a reason for hiding this comment

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

I can try if there's an outline of how to reintroduce it. I have not much experience on arraycontexts, so I have no idea what needs to be done.

Yeah, I don't have a good overview either, so that probably needs some discussion. There's a profiling array context here
https://github.com/illinois-ceesd/mirgecom/blob/f9cf0f3557b7d5c89f58e8362ebf50820261219f/mirgecom/profiling.py
but not sure if a similar interface will work for gathering timings on the FMM.

Yes, I use it to get results for a paper.

Ok, then definitely either revert or hold off on merging.

At the moment, the timing code and the manual event handling are not particularly compatible with the arraycontext stuff (as it is) and removing both is pretty breaking.

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