Skip to content

Fix TaggableCLArray copy without queue - #147

Merged
inducer merged 1 commit into
inducer:mainfrom
alexfikl:fix-taggable-copy
Mar 3, 2022
Merged

Fix TaggableCLArray copy without queue#147
inducer merged 1 commit into
inducer:mainfrom
alexfikl:fix-taggable-copy

Conversation

@alexfikl

@alexfikl alexfikl commented Feb 20, 2022

Copy link
Copy Markdown
Collaborator

No description provided.

Comment thread arraycontext/impl/pyopencl/taggable_cl_array.py Outdated
@alexfikl
alexfikl marked this pull request as ready for review February 20, 2022 02:29
@alexfikl
alexfikl marked this pull request as draft February 20, 2022 02:38
@alexfikl alexfikl changed the title Fix TaggableCLArray copy with queue Fix TaggableCLArray copy without queue Feb 20, 2022
@alexfikl
alexfikl marked this pull request as ready for review February 20, 2022 03:10
Comment thread arraycontext/impl/pyopencl/taggable_cl_array.py
@alexfikl
alexfikl marked this pull request as draft February 20, 2022 04:23
@alexfikl
alexfikl marked this pull request as ready for review February 20, 2022 17:14
@inducer

inducer commented Feb 21, 2022

Copy link
Copy Markdown
Owner

So I think the underlying issue here is that there's confusion on what copy should do. (Which I should have caught before merging #111, sorry!)

  • Taggable assumes copy can be used for (cheap-ish, immutable) return-with-modifications.
  • cla.Array says copy copies array data.

IMO:

  • It's not possible to cleanly untangle those two use cases without causing un-obvious corner cases.
  • TaggableCLArray should not override copy, and Taggable should not assume copy is used to apply tags. The latter makes sense for another reason: If the Taggable is a dataclass, then the correct spelling would be something with replace.

So my vote would be to give Taggable an interface (_with_tags) that a subclass needs to define, with a deprecated fallback to using copy.

What do the both of you think?

@alexfikl

alexfikl commented Feb 21, 2022

Copy link
Copy Markdown
Collaborator Author

What do the both of you think?

I think that's mostly what the current version does, except that copy still tries to handle tags. I can remove that once we agree on an approach.

For me this is 👍.

EDIT: Not super worried about deprecating things though, since this was just introduced and nobody uses it outside arraycontext (right?).

@inducer

inducer commented Feb 21, 2022

Copy link
Copy Markdown
Owner

inducer/pytools#120 realizes part of this.

EDIT: Not super worried about deprecating things though, since this was just introduced and nobody uses it outside arraycontext (right?).

I'm talking about changing Taggable, and that has uses throughout pytato, here, and loopy.

@alexfikl

Copy link
Copy Markdown
Collaborator Author

I'm talking about changing Taggable, and that has uses throughout pytato, here, and loopy.

Ah! I read that as a shorthand for TaggableCLArray and got confused. Then yeah, completely agree that needs a bit more care. Thanks for taking care of the pytools side!

@alexfikl
alexfikl force-pushed the fix-taggable-copy branch 5 times, most recently from 8221b57 to fd1ad02 Compare March 3, 2022 02:31
@alexfikl

alexfikl commented Mar 3, 2022

Copy link
Copy Markdown
Collaborator Author

@inducer This needs a pytools release that includes inducer/pytools#120. Otherwise it should be good for another look!

@inducer

inducer commented Mar 3, 2022

Copy link
Copy Markdown
Owner

https://pypi.org/project/pytools/2022.1.1/

@inducer
inducer force-pushed the fix-taggable-copy branch from fd1ad02 to 9b4eccb Compare March 3, 2022 19:34
@inducer

inducer commented Mar 3, 2022

Copy link
Copy Markdown
Owner

Thx! Pushed a pytools version constraint to (hopefully) make mirgecom happy, and underscored unwrap_cl_array. LGTM, in it goes, pending CI. :)

@inducer
inducer enabled auto-merge (rebase) March 3, 2022 19:36
@alexfikl

alexfikl commented Mar 3, 2022

Copy link
Copy Markdown
Collaborator Author

Thx! Pushed a pytools version constraint to (hopefully) make mirgecom happy, and underscored unwrap_cl_array. LGTM, in it goes, pending CI. :)

Looks like it picked it up! Thank you.

@inducer
inducer merged commit 6a6be64 into inducer:main Mar 3, 2022
@alexfikl
alexfikl deleted the fix-taggable-copy branch March 3, 2022 20: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.

3 participants