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

Record: make field ordering deterministic, add test #199

Merged
merged 10 commits into from
Mar 20, 2024

Conversation

matthiasdiener
Copy link
Contributor

@matthiasdiener matthiasdiener commented Feb 27, 2024

This is a proof-of-concept and is ready for a first look @inducer. I'm not sure if there is a better way to do this.

Please squash

@matthiasdiener matthiasdiener changed the title Record: make field ordering deterministic Record: make field ordering deterministic, add test Feb 27, 2024
@inducer
Copy link
Owner

inducer commented Feb 27, 2024

  • This seems OK, although it is a compatibility break, which I'm not loving.
  • We should just deprecate the thing for removal in 2025. I.e. have it spew warnings, and remove it from the docs. (in favor of dataclasses, which do the same thing, but more competently)

@matthiasdiener
Copy link
Contributor Author

  • This seems OK, although it is a compatibility break, which I'm not loving.

Is there a use case that breaks with this change? My goal was to remain compatible with the previous set-based implementation (which is the reason for the conversion from set to dict in __init__).

  • We should just deprecate the thing for removal in 2025. I.e. have it spew warnings, and remove it from the docs. (in favor of dataclasses, which do the same thing, but more competently)

What do you think of c32e6d0?

@matthiasdiener matthiasdiener marked this pull request as ready for review February 27, 2024 21:50
@matthiasdiener
Copy link
Contributor Author

matthiasdiener commented Feb 27, 2024

Setting this as ready for review. In addition to the pytato/loopy tests, mirgecom also seems to run fine with it.

pytools/__init__.py Outdated Show resolved Hide resolved
pytools/__init__.py Outdated Show resolved Hide resolved
@matthiasdiener matthiasdiener force-pushed the record-dict branch 2 times, most recently from 5905e52 to e0e04b0 Compare March 15, 2024 21:01
@inducer inducer force-pushed the record-dict branch 3 times, most recently from 12fd003 to 9cec923 Compare March 20, 2024 18:18
@inducer inducer merged commit 0b4604c into inducer:main Mar 20, 2024
16 checks passed
inducer added a commit that referenced this pull request Mar 20, 2024
@inducer
Copy link
Owner

inducer commented Mar 20, 2024

I'll be honest, I underestimated the compatibility impact of this. I've just pushed a revert.

@inducer
Copy link
Owner

inducer commented Mar 20, 2024

inducer/loopy#785 gets rid of a good bunch more Records in Loopy, which may make this more plausible.

@inducer
Copy link
Owner

inducer commented Mar 20, 2024

@matthiasdiener Could you please re-make a PR for this?

@matthiasdiener
Copy link
Contributor Author

Sure, no problem! Sorry for the confusion. Is there a particular error that came up with this?

@inducer
Copy link
Owner

inducer commented Mar 20, 2024

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