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

Compatibility with (future) pymbolic 2024.1 #27

Closed
inducer opened this issue Nov 3, 2024 · 7 comments · Fixed by #28
Closed

Compatibility with (future) pymbolic 2024.1 #27

inducer opened this issue Nov 3, 2024 · 7 comments · Fixed by #28

Comments

@inducer
Copy link
Contributor

inducer commented Nov 3, 2024

@connorjward Could you guys pull inducer/loopy@7ac9fa6 into Firedrake? pymbolic is going through a bit of a remodel, with inducer/pymbolic#152 and inducer/pymbolic#125.

The linked version will be compatible with both old-timey and new-timey pymbolic. New-timey pymbolic is mostly inducer/pymbolic#152, which, in the near future, will become pymbolic 2024.1. As a side note, pymbolic 2024.1 will require Python 3.10. Loopy will likely follow suit in the near future. I know you all are still targeting 3.9, so if that's an issue, let's discuss.

(The currently-broken mypy in Loopy is the result of more precise typing information from inducer/pymbolic#125, in case you're wondering.)

Things are generally compatible, however inducer/pymbolic#152 introduces one long-overdue behavior change: 0 + x is now Sum(0, x), not x. Call pymbolic.flatten to get x. This change should be semantics-neutral, and AFAIK nothing in Loopy actually cares. Nonetheless, some of loopy's tests want specific forms of some expressions, so the linked version of loopy sprinkles some calls to flatten.

@connorjward
Copy link
Contributor

Thanks for the heads up @inducer. I will mention the 3.10 concerns with the group this Wednesday and let you know.

@connorjward
Copy link
Contributor

One issue I've just encountered is until now we have been using the latest release of pymbolic (2022.2). This breaks with current loopy main. Therefore if we want to update loopy but retain 3.9 compatibility we need to stop using a versioned release of pymbolic.

inducer added a commit to inducer/loopy that referenced this issue Nov 6, 2024
This restores compatibility with pymbolic 2022.2

x-ref: firedrakeproject/loopy#27
inducer added a commit to inducer/loopy that referenced this issue Nov 6, 2024
This restores compatibility with pymbolic 2022.2

x-ref: firedrakeproject/loopy#27
@inducer
Copy link
Contributor Author

inducer commented Nov 6, 2024

Thanks for flagging this, that was an (unnecessary) oversight. AFAICT, inducer/loopy#874 restores compatibility with pymbolic 2022.2, so the updated suggestion would be to pull in whatever that becomes.

@inducer
Copy link
Contributor Author

inducer commented Nov 6, 2024

As a side note, pymbolic 2024.1 will require Python 3.10. Loopy will likely follow suit in the near future. I know you all are still targeting 3.9, so if that's an issue, let's discuss.

The driving factor behind this is that mappers now inherit from Generic, and XYZMapper(Mapper[ExpressionT, []]) only becomes allowed syntax in 3.10. It may be possible to hack around this in a 3.9-compatible manner, but I saw no obvious way to maintain compatibility with 3.9 and enabling type-checking at acceptable cost. (One could copy-paste the class body and hide the 3.9 version under if not TYPE_CHECKING?)

inducer added a commit to inducer/loopy that referenced this issue Nov 6, 2024
This restores compatibility with pymbolic 2022.2

x-ref: firedrakeproject/loopy#27
@inducer
Copy link
Contributor Author

inducer commented Nov 6, 2024

the updated suggestion would be to pull in whatever that becomes.

inducer/loopy@da84302

@connorjward
Copy link
Contributor

Thanks for this. I will refresh the loopy update PR to see if things now work and get that merged shortly.

Following some discussion in today's meeting we have decided that we are happy to drop support for 3.9 at the same time as you.

@connorjward
Copy link
Contributor

@inducer just to let you know that the version of loopy we just merged is in fact not compatible with Python 3.9: #28 (comment).

This is fine for us as we agreed to shift to 3.10+ anyway.

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 a pull request may close this issue.

2 participants