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

Revert "Hack dataclass'd function_interface to avoid breaking Firedrake" #886

Merged
merged 1 commit into from
Dec 3, 2024

Conversation

inducer
Copy link
Owner

@inducer inducer commented Nov 25, 2024

This reverts commit 1af4523.

@inducer
Copy link
Owner Author

inducer commented Nov 25, 2024

@connorjward I would like to revert this commit that adds a bunch of atrocities on top of a recent refactor that makes the loopy callables into dataclasses. All of loopy works OK without the atrociities (see the CI). The atrocities are being committed in order to keep Firedrake working. The one I like perhaps the least is that the dataclasses cannot be frozen (immutable), mainly because of this line:

https://github.com/OP2/PyOP2/blob/31471a606a852aed250b05574d1fc2a2874eec31/pyop2/codegen/rep2loopy.py#L119

Replacing that line with

object.__setattr__(self, "name_in_target", name_in_target if name_in_target else self.name)

would already unblock things a bit.

Beyond that, it'd be nice if all that extra code (see the diff in this PR) could go. For that, Firedrake's derived loopy callables would have to become dataclasses. One notable constraint is that frozen dataclasses cannot inherit from non-frozen, so the transition to frozen would likely have to happen first.

As you can tell, all this requires a bit of coordination. I'm hoping you're open to serving as the point of contact here, but feel free to LMK if someone else would be better. Thanks!

@inducer inducer mentioned this pull request Nov 25, 2024
1 task
@connorjward
Copy link
Contributor

Thanks @inducer for the heads up and detailed information. I will hopefully find the time to address this by the end of the week.

Note that we are currently moving PyOP2 into the Firedrake repository so this fix will have to wait until that is merged.

@connorjward
Copy link
Contributor

Fixing this is now quite a high priority. Turns out that what you have here isn't quite sufficient for us. Specifically _all_attrs() needs to be ordered for us to have consistent hashes across ranks.

I will try to get things working on this branch.

@connorjward
Copy link
Contributor

I am currently working on firedrakeproject/firedrake#3898 which uses this branch.

inducer added a commit that referenced this pull request Nov 29, 2024
@inducer
Copy link
Owner Author

inducer commented Nov 29, 2024

Fixing this is now quite a high priority. Turns out that what you have here isn't quite sufficient for us. Specifically _all_attrs() needs to be ordered for us to have consistent hashes across ranks.

Whoops! Like this: #892?

@connorjward
Copy link
Contributor

Fixing this is now quite a high priority. Turns out that what you have here isn't quite sufficient for us. Specifically _all_attrs() needs to be ordered for us to have consistent hashes across ranks.

Whoops! Like this: #892?

Yep. But I think instead of that we can just remove the Firedrake-specific code as this PR does. I have pretty much everything passing using this branch. I think you are welcome to just merge this and I will fix things up our end next week.

@inducer inducer force-pushed the revert-hack-for-firedrake branch from ae8db5b to 4928fc9 Compare December 3, 2024 15:22
@inducer inducer marked this pull request as ready for review December 3, 2024 16:14
@inducer
Copy link
Owner Author

inducer commented Dec 3, 2024

Merging despite Firedrake failures as instructed by @connorjward here.

@inducer inducer merged commit 76f4afc into main Dec 3, 2024
17 of 18 checks passed
@inducer inducer deleted the revert-hack-for-firedrake branch December 3, 2024 16:15
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