Repository navigation
BUG: missing support for Python optimized mode #17471
Description
Description
The regression I'm seeing is clearly from #17118, but wasn't spotted because we don't have a test job for it. Fixing the crash itself should be simple but we should probably add some sort of regular testing for it, probably in weekly-cron.
Expected behavior
No response
How to Reproduce
$ python -OO -c "from astropy.io import *"Traceback (most recent call last):
File "<string>", line 1, in <module>
File "/Users/clm/dev/astropy-project/coordinated/astropy/astropy/__init__.py", line 23, in <module>
from . import config as _config
File "/Users/clm/dev/astropy-project/coordinated/astropy/astropy/config/__init__.py", line 8, in <module>
from .configuration import *
File "/Users/clm/dev/astropy-project/coordinated/astropy/astropy/config/configuration.py", line 27, in <module>
from .paths import get_config_dir_path
File "/Users/clm/dev/astropy-project/coordinated/astropy/astropy/config/paths.py", line 78, in <module>
get_config_dir_path.__doc__
TypeError: unsupported operand type(s) for +: 'NoneType' and 'str'
Versions
platform
--------
platform.platform() = 'macOS-15.1.1-arm64-arm-64bit'
platform.version() = 'Darwin Kernel Version 24.1.0: Thu Oct 10 21:03:11 PDT 2024; root:xnu-11215.41.3~2/RELEASE_ARM64_T6020'
platform.python_version() = '3.11.10'
packages
--------
astropy 7.1.dev202+g5d51416f46
numpy 2.1.3
scipy 1.14.1
matplotlib 3.9.2
pandas 2.2.3
pyerfa 2.0.1.5
for reference, progress is being tracked on the linked PR #17472
Activity
[-]BUG: (7.0.0 regression) some subpackages cannot import under Python's optimized mode[/-][+]BUG: missing support for Python optimized mode[/+]neutrinoceros commentedon Nov 29, 2024
I broaden the scope of the issue via the title as I'm discovering many (small) problems, most not new, with running astropy under
python -OO. Several PRs will be needed, I think.neutrinoceros commentedon Nov 29, 2024
Looks like this already happened before: #15028
eerovaher commentedon Nov 29, 2024
The Python optimization levels are a mechanism that allows users to disable
assertstatements and docstrings. We cannot prevent users from setting the optimization levels, so usingassertstatements in business logic or relying on the existence of docstrings can cause bugs.The Ruff rule S101 (assert) guards against
assertstatements. We are currently not enforcing it, but we really should:astropy/.ruff.toml
Line 144 in 7c800f8
We already have a well-justified exception for tests:
astropy/pyproject.toml
Lines 399 to 403 in 7c800f8
The potentially invalid uses of docstrings can be guarded against with type checking because the type of a docstring is
str | None, so type checkers wont let theNonecase to go unnoticed. Admittedly, we can't rely on that untilastropyis fully annotated and at the rate we are annotating the code this will take many decades.mhvk commentedon Nov 29, 2024
Agreed that we can turn on that
ruffrule. For the presumably very few cases where it is just to help indicate expectations, one can add anoqa. Let's not replace things withraise AssertionError, though!neutrinoceros commentedon Nov 29, 2024
I think assert statements in the code base are okay as long as they are not an integral part of business logic.
In fact, running
ruff check astropy --select S101gets you 41 violations as of yesterday'smainbranch; but besides the 3 that are already being addressed in #17476 and #17477, I only find one that's apparently relied on, though it doesn't cause any test to fail if one removes it:astropy/astropy/units/quantity.py
Line 1357 in b008f98
I'm not sure it's worth all the work needed to actually enforce this rule. We should already be spotting the important problems by running the test suite in optimized mode, as is proposed in #17472
eerovaher commentedon Nov 29, 2024
If there are
assertstatements in our code that don't cause test failures with optimizations enabled then that means either our tests are not as complete as they should be or there is dead code we should remove.WilliamJamieson commentedon Nov 29, 2024
I agree that we enforce S101, but we will need to exclude all the test files.
neutrinoceros commentedon Nov 29, 2024
True, but either way could be significant amount of work with unclear gains. To be clear, if there's a consensus that it's worth doing, I'm okay to go with it, I'm just worried that coding purity may not be a consensual priority to the project.
it's not necessary, this rule already ignores test files.
mhvk commentedon Nov 29, 2024
That
unitsone should definitely be rewritten (and a test added trying to get the index for a non-dimensionless quantity). Currently, on astropy 5.2.1 (where runningpython -OOis OK...):The last one raises
TypeErrorwithout-OO.This example does make me think it is worthwhile to check that the assert we use are actually not important for the logic...
neutrinoceros commentedon Nov 29, 2024
Thanks; I'm adding the missing test (and fix) in #17481
pllim commentedon Dec 9, 2024
Is this a duplicate of #15052 ?
neutrinoceros commentedon Dec 9, 2024
More of an extension and a revival, since there were no systematic check introduced and most problems I caught while hunting this actually predate your issue.
pllim commentedon Dec 9, 2024
Maybe I should close the other one as duplicate then. Thanks!
marscode-agent commentedon Dec 11, 2024