-
Notifications
You must be signed in to change notification settings - Fork 473
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
Use dataclass(frozen=True) for compatibility with Python 3.13 (incomplete, needs help) #2037
Conversation
Interesting test failure from
|
I'm a bit stuck on what to do next with this one. |
CodSpeed Performance ReportMerging #2037 will not alter performanceComparing Summary
|
While I have been a long proponent of making exception immutable (i.e. frozen when using a dataclasses) this has bit me many times. Do you know what is the change in Python 3.13 that makes this required? |
See: https://github.com/python/cpython/blame/3.13/Lib/dataclasses.py#L1035-L1044 |
I've tried to play with it, but haven't found any good solution with keeping the dataclasses frozen in errors.py. Even if you use functools.cached_property, or other way to avoid mutation within, the errors will eventually get mutated, setting I'm not experienced enough to be able to help here, sorry. |
@hgrecco would it be ok to move them all to |
I have finally figured out the issue. The issue is with double inheriting with Lines 133 to 134 in 2839f6e
https://github.com/python/cpython/blob/3024b16d51bb7f74177c5a5038cc9a56fd2b26bd/Objects/exceptions.c#L2400-L2404 Removing that inheritance made the sphinx tests pass, but most likely that's not what we want here |
@@ -130,7 +130,7 @@ def __reduce__(self): | |||
return self.__class__, tuple(getattr(self, f.name) for f in fields(self)) | |||
|
|||
|
|||
@dataclass(frozen=False) | |||
@dataclass(frozen=True) | |||
class UndefinedUnitError(AttributeError, PintError): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class UndefinedUnitError(AttributeError, PintError): | |
class UndefinedUnitError(PintError): |
This technically would make the tests pass and as far as I was able to dig, this is the only problematic constructor? All of the others do not seem to have any constructor arguments
I tried to poke around to see if there is anything here I could help with.
@simonw, I wonder whether, if you were to rebase the branch and update those latter two files to Python 3.12, the CI error would disappear. My guess is:
If you lack time to do this, please let me know and I can open a superseding PR with those suggested changes. |
@khaeru Run the |
Running this locally, I see the failures when un-pickling the exceptions via tblib with restores some extra It looks like this is a dependency of I think we either need to make |
@tacaswell I think you are experiencing a different issue. Either it is an issue with the other @hgrecco gentle ping |
My judgement is that removing try:
some_code_that_raises_UndefinedUnitError()
except AttributeError:
... would be broken. |
Yes, but realistically, why would you |
Why users do things does not really come into what is or is not an API break. However, a reason I would want to use the base exception rather than the pint-specific exception is if you are developing a packaging that can use multiple unit libraries where pint is optional and you can use |
Sure, but to my knowledge, there is no such multi-library spec/interface that actually exists to specify this. So IMO this goes squarely in the box of "nice to have", which is firmly below "currently broken on Python 3.13". Also, pint is versioned 0.x, so major breaks, while considered carefully, are seemingly allowed. If fixing tblib is an option, then great. Right now, pint and anything that depends on it is currently unimportable on the latest version of Python and we really need to get this fixed, one way or another. |
diff --git a/pint/errors.py b/pint/errors.py
index f080f52..a1eeffc 100644
--- a/pint/errors.py
+++ b/pint/errors.py
@@ -103,7 +103,7 @@ class DefinitionError(ValueError, PintError):
@dataclass(frozen=True)
-class DefinitionSyntaxError(ValueError, PintError):
+class DefinitionSyntaxError(ValueError):
"""Raised when a textual definition has a syntax error."""
msg: str
@@ -116,7 +116,7 @@ class DefinitionSyntaxError(ValueError, PintError):
@dataclass(frozen=True)
-class RedefinitionError(ValueError, PintError):
+class RedefinitionError(ValueError):
"""Raised when a unit or prefix is redefined."""
name: str
@@ -131,7 +131,7 @@ class RedefinitionError(ValueError, PintError):
@dataclass(frozen=True)
-class UndefinedUnitError(AttributeError, PintError):
+class UndefinedUnitError(AttributeError):
"""Raised when the units are not defined in the unit registry."""
unit_names: str | tuple[str, ...]
@@ -151,7 +151,7 @@ class UndefinedUnitError(AttributeError, PintError):
@dataclass(frozen=True)
-class PintTypeError(TypeError, PintError):
+class PintTypeError(TypeError):
def __reduce__(self):
return self.__class__, tuple(getattr(self, f.name) for f in fields(self))
@@ -234,7 +234,7 @@ class LogarithmicUnitCalculusError(PintTypeError):
@dataclass(frozen=True)
-class UnitStrippedWarning(UserWarning, PintError):
+class UnitStrippedWarning(PintError):
msg: str
def __reduce__(self):
@@ -248,7 +248,7 @@ class UnexpectedScaleInContainer(Exception):
@dataclass(frozen=True)
-class UndefinedBehavior(UserWarning, PintError):
+class UndefinedBehavior(PintError):
msg: str
def __reduce__(self):
Even with this patch I am still seeing the pickle failures with tblib (which is expected because it registers on Dropping all of the |
You can make changes to flexparser. It used to be part of pint but was
split out a few versions ago.
…On Wed, 16 Oct 2024, 23:54 Thomas A Caswell, ***@***.***> wrote:
diff --git a/pint/errors.py b/pint/errors.py
index f080f52..a1eeffc 100644--- a/pint/errors.py+++ b/pint/errors.py@@ -103,7 +103,7 @@ class DefinitionError(ValueError, PintError):
@DataClass(frozen=True)-class DefinitionSyntaxError(ValueError, PintError):+class DefinitionSyntaxError(ValueError):
"""Raised when a textual definition has a syntax error."""
msg: str@@ -116,7 +116,7 @@ class DefinitionSyntaxError(ValueError, PintError):
@DataClass(frozen=True)-class RedefinitionError(ValueError, PintError):+class RedefinitionError(ValueError):
"""Raised when a unit or prefix is redefined."""
name: str@@ -131,7 +131,7 @@ class RedefinitionError(ValueError, PintError):
@DataClass(frozen=True)-class UndefinedUnitError(AttributeError, PintError):+class UndefinedUnitError(AttributeError):
"""Raised when the units are not defined in the unit registry."""
unit_names: str | tuple[str, ...]@@ -151,7 +151,7 @@ class UndefinedUnitError(AttributeError, PintError):
@DataClass(frozen=True)-class PintTypeError(TypeError, PintError):+class PintTypeError(TypeError):
def __reduce__(self):
return self.__class__, tuple(getattr(self, f.name) for f in fields(self))
@@ -234,7 +234,7 @@ class LogarithmicUnitCalculusError(PintTypeError):
@DataClass(frozen=True)-class UnitStrippedWarning(UserWarning, PintError):+class UnitStrippedWarning(PintError):
msg: str
def __reduce__(self):@@ -248,7 +248,7 @@ class UnexpectedScaleInContainer(Exception):
@DataClass(frozen=True)-class UndefinedBehavior(UserWarning, PintError):+class UndefinedBehavior(PintError):
msg: str
def __reduce__(self):
Even with this patch I am still seeing the pickle failures with tblib
(which is expected because it registers on Exception).
Dropping all of the frozen=True does not work because it looks like
errors from flexparser use frozen dataclasses and are mixed into the pint
classes.
—
Reply to this email directly, view it on GitHub
<#2037 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADEMLEFWHDXFU4Y6ROAJNDDZ33VDZAVCNFSM6AAAAABKZYNZ7WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMJYGEYDGMRXHE>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
I should start by apologizing from showing up randomly and having a bunch of Strong Opinions, but hopefully Ryan will vouch for me ;) Looking at After a very quick skim of the code I have a couple of design questions (which maybe better as an issue on flexparser, but keeping here as this is the driver for asking these questions) Do the error classes have to be both a A second concern is that while |
No worries, and you are right about considering these API breakages. I was also considering unfreezing the About your previous patch, the main issue is with If we want to properly fix the Edit: I've hit a brick wall. It seems that exception chaining cannot be used with try:
try:
raise TypeError('Something awful has happened')
except TypeError as e:
raise ValueError('There was a bad value') from e
# This works because it checks only the last exception thrown
except ValueError as e:
print(e.__context__)
# This does not work because it does not catch other exceptions in the chain
# except TypeError as e:
# print(e.__context__) |
This I think is a good question:
The answer is no. I like the idea of immutability for Statement and ParsedStatment. But maybe an Exception could include as an attribute an immutable Statement. First, thank you all for the late reply. This bug is due to my love of immutable objects. But I might have overstepped myself here, or at least Python thinks I have. As far as I can tell, there are the following solutions:
All are problematic in a sense.
My gut feeling is that (3) is more work for devs, but better for the user Opinions? |
Ultimately any approach is welcome as long as it unblocks this issue and it seems like the user would have a surprise regardless of the solution. Also worth considering that pint is still not in a stable semver and many impacted users are in this thread.
For the user these would be equivalent and as long as there are no other major breaking changes that would have the user freeze or upper-bound either
I need to investigate more carefully when I'm at the computer, but it seems this case is very limited (only 1 true case in sourcegraph search) and even there the code readability would be improved by switching to except Indeed this would be a surprising breaking change, but I think it would be a preferred design for the long run, i.e. for the user to be more selective on the error source. Catching Ultimately this is a design question of how should a library catch and raise their own, third-party and built-in exceptions. I'm still struggling to think about this one, but one design would be to always transform the exceptions to your own type with |
I just merge a PR in Flexparser to undataclass exceptions. If there are no showstoppers, tomorrow I will try to try it here. |
fixed now |
I am still seeing issues:
with the main branch of both flexparser and pint:
|
Can't really replicate it: $ python3.13 -m venv venv-3.13
$ source venv-3.13/bin/activate
(venv-3.13) lecris@Tuxedo:~/tmp/python$ pip install pint
Collecting pint
Downloading Pint-0.24.4-py3-none-any.whl.metadata (8.5 kB)
Collecting platformdirs>=2.1.0 (from pint)
Downloading platformdirs-4.3.6-py3-none-any.whl.metadata (11 kB)
Collecting typing-extensions>=4.0.0 (from pint)
Using cached typing_extensions-4.12.2-py3-none-any.whl.metadata (3.0 kB)
Collecting flexcache>=0.3 (from pint)
Downloading flexcache-0.3-py3-none-any.whl.metadata (7.0 kB)
Collecting flexparser>=0.4 (from pint)
Downloading flexparser-0.4-py3-none-any.whl.metadata (18 kB)
Downloading Pint-0.24.4-py3-none-any.whl (302 kB)
Downloading flexcache-0.3-py3-none-any.whl (13 kB)
Downloading flexparser-0.4-py3-none-any.whl (27 kB)
Downloading platformdirs-4.3.6-py3-none-any.whl (18 kB)
Using cached typing_extensions-4.12.2-py3-none-any.whl (37 kB)
Installing collected packages: typing-extensions, platformdirs, flexparser, flexcache, pint
Successfully installed flexcache-0.3 flexparser-0.4 pint-0.24.4 platformdirs-4.3.6 typing-extensions-4.12.2
[notice] A new release of pip is available: 24.2 -> 24.3.1
[notice] To update, run: pip install --upgrade pip
(venv-3.13) $ python -c "import pint" I've also tried the original issue here with |
Have you tried upgrading to Pint 0.24.4? |
🐑 thought I had master checked out, but I was still on the branch from this PR (+ some local changes). Sorry for the noise and thank you for fixing the issue! |
pre-commit run --all-files
with no errors