search

ColwynGulliford / distgen

Particle distribution generator
https://colwyngulliford.github.io/distgen/
Apache License 2.0
13 stars 8 forks source link

distgen breaking python 3.9 tests. #22

Closed MichaelEhrlichman closed 1 month ago

MichaelEhrlichman commented 1 month ago

distgen has been breaking our python 3.9 tests. python 3.10, 3.11, and 3.12 tests run OK. The test output is copied below

============================= test session starts ==============================
platform linux -- Python 3.9.20, pytest-8.3.3, pluggy-1.5.0 -- /home/runner/miniconda3/envs/lcls-lattice-dev/bin/python3.9
cachedir: .pytest_cache
rootdir: /home/runner/work/lcls-lattice/lcls-lattice
plugins: anyio-4.6.0
collecting ... collected 28 items / 1 error

==================================== ERRORS ====================================
_____________ ERROR collecting python/tests/test_distgen_models.py _____________
python/tests/test_distgen_models.py:1: in <module>
    from distgen import Generator
../../../miniconda3/envs/lcls-lattice-dev/lib/python3.9/site-packages/distgen/__init__.py:1: in <module>
    from .generator import Generator
../../../miniconda3/envs/lcls-lattice-dev/lib/python3.9/site-packages/distgen/generator.py:13: in <module>
    from . import archive
../../../miniconda3/envs/lcls-lattice-dev/lib/python3.9/site-packages/distgen/archive.py:4: in <module>
    from .tools import isotime, flatten_dict, unflatten_dict
../../../miniconda3/envs/lcls-lattice-dev/lib/python3.9/site-packages/distgen/tools.py:14: in <module>
    import pydicom as dicom
../../../miniconda3/envs/lcls-lattice-dev/lib/python3.9/site-packages/pydicom/__init__.py:31: in <module>
    from pydicom.dataelem import DataElement
../../../miniconda3/envs/lcls-lattice-dev/lib/python3.9/site-packages/pydicom/dataelem.py:17: in <module>
    from pydicom import config  # don't import datetime_conversion directly
../../../miniconda3/envs/lcls-lattice-dev/lib/python3.9/site-packages/pydicom/config.py:407: in <module>
    import pydicom.pixel_data_handlers.numpy_handler as np_handler  # noqa
../../../miniconda3/envs/lcls-lattice-dev/lib/python3.9/site-packages/pydicom/pixel_data_handlers/__init__.py:5: in <module>
    from pydicom.misc import warn_and_log
../../../miniconda3/envs/lcls-lattice-dev/lib/python3.9/site-packages/pydicom/misc.py:23: in <module>
    def size_in_bytes(expr: int | float | str | None) -> None | float | int:
E   TypeError: unsupported operand type(s) for |: 'type' and 'type'
=============================== warnings summary ===============================
../../../miniconda3/envs/lcls-lattice-dev/lib/python3.9/site-packages/distgen/physical_constants.py:44
../../../miniconda3/envs/lcls-lattice-dev/lib/python3.9/site-packages/distgen/physical_constants.py:44
  /home/runner/miniconda3/envs/lcls-lattice-dev/lib/python3.9/site-packages/distgen/physical_constants.py:44: ConstantWarning: Constant 'tau mass energy equivalent in MeV' is not in current CODATA 2018 data set
    return scipy.constants.value(key)*unit_registry(scipy.constants.unit(key))

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================== short test summary info ============================
ERROR python/tests/test_distgen_models.py - TypeError: unsupported operand type(s) for |: 'type' and 'type'
!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
========================= 2 warnings, 1 error in 1.97s =========================
ColwynGulliford commented 1 month ago

Thanks @MichaelEhrlichman. Is this causing headaches and needs resolving quickly? I tried reproducing with Python 3.9, but don't get the same issue. Here I made a Conda Env with python=3.9, but installed the distgen source from the repo as an editable package via pip install -e .

I was hoping to see the problem that way since then I can debug using the repo source before making a release.

(distgen_py39) colwyngulliford@Colwyns-MBP-2 distgen % pytest ========================================================================= test session starts ========================================================================= platform darwin -- Python 3.9.20, pytest-8.3.3, pluggy-1.5.0 rootdir: /Users/colwyngulliford/GitHub/distgen collected 616 items

tests/test_beam.py ............................................................................................................................................ [ 22%] ............................................................................................................................................................... [ 48%] ............................................................................................................................................................... [ 74%] ........................................................................................................................................ [ 96%] tests/test_dist.py ..................... [ 99%] tests/test_import.py . [100%]

========================================================================== warnings summary =========================================================================== physical_constants.py:44 physical_constants.py:44 /Users/colwyngulliford/GitHub/distgen/distgen/physical_constants.py:44: ConstantWarning: Constant 'tau mass energy equivalent in MeV' is not in current CODATA 2018 data set return scipy.constants.value(key)*unit_registry(scipy.constants.unit(key))

distgen/tests/test_dist.py::test_1d_pdf_from_file /Users/colwyngulliford/GitHub/distgen/distgen/dist.py:267: RuntimeWarning: More than 20 figures have been opened. Figures created through the pyplot interface (matplotlib.pyplot.figure) are retained until explicitly closed and may consume too much memory. (To control this warning, see the rcParam figure.max_open_warning). Consider using matplotlib.pyplot.close(). plt.figure()

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html ================================================================== 616 passed, 3 warnings in 16.52s ===================================================================

ColwynGulliford commented 1 month ago

Ok scratch that, I was able to reproduce using a pure conda installation:

distgen/tools.py:14: in <module>
    import pydicom as dicom
../../miniforge3/envs/distgen_py39/lib/python3.9/site-packages/pydicom/__init__.py:31: in <module>
    from pydicom.dataelem import DataElement
../../miniforge3/envs/distgen_py39/lib/python3.9/site-packages/pydicom/dataelem.py:17: in <module>
    from pydicom import config  # don't import datetime_conversion directly
../../miniforge3/envs/distgen_py39/lib/python3.9/site-packages/pydicom/config.py:407: in <module>
    import pydicom.pixel_data_handlers.numpy_handler as np_handler  # noqa
../../miniforge3/envs/distgen_py39/lib/python3.9/site-packages/pydicom/pixel_data_handlers/__init__.py:5: in <module>
    from pydicom.misc import warn_and_log
../../miniforge3/envs/distgen_py39/lib/python3.9/site-packages/pydicom/misc.py:23: in <module>
    def size_in_bytes(expr: int | float | str | None) -> None | float | int:
E   TypeError: unsupported operand type(s) for |: 'type' and 'type'
========================================================================== warnings summary ===========================================================================
distgen/physical_constants.py:44
distgen/physical_constants.py:44
  /Users/colwyngulliford/GitHub/distgen/distgen/physical_constants.py:44: ConstantWarning: Constant 'tau mass energy equivalent in MeV' is not in current CODATA 2018 data set
    return scipy.constants.value(key)*unit_registry(scipy.constants.unit(key))

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
======================================================================= short test summary info =======================================================================
ERROR distgen/tests - TypeError: unsupported operand type(s) for |: 'type' and 'type'
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
==================================================================== 2 warnings, 1 error in 19.64s ====================================================================

From what I can tell it looks like some weird issue with the package pydicom. Just importing Pydicom using the same Python 3.9 Env, that package crashes on its own with the same error, so its not a native Distgen thing:

(distgen_py39) colwyngulliford@Colwyns-MBP-2 distgen % python
Python 3.9.20 | packaged by conda-forge | (main, Sep 22 2024, 14:06:09) 
[Clang 17.0.6 ] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import pydicom as dicom
Traceback (most recent call last):
 File "<stdin>", line 1, in <module>
 File "/Users/colwyngulliford/miniforge3/envs/distgen_py39/lib/python3.9/site-packages/pydicom/__init__.py", line 31, in <module>
  from pydicom.dataelem import DataElement
 File "/Users/colwyngulliford/miniforge3/envs/distgen_py39/lib/python3.9/site-packages/pydicom/dataelem.py", line 17, in <module>
  from pydicom import config # don't import datetime_conversion directly
 File "/Users/colwyngulliford/miniforge3/envs/distgen_py39/lib/python3.9/site-packages/pydicom/config.py", line 407, in <module>
  import pydicom.pixel_data_handlers.numpy_handler as np_handler # noqa
 File "/Users/colwyngulliford/miniforge3/envs/distgen_py39/lib/python3.9/site-packages/pydicom/pixel_data_handlers/__init__.py", line 5, in <module>
  from pydicom.misc import warn_and_log
 File "/Users/colwyngulliford/miniforge3/envs/distgen_py39/lib/python3.9/site-packages/pydicom/misc.py", line 23, in <module>
  def size_in_bytes(expr: int | float | str | None) -> None | float | int:
TypeError: unsupported operand type(s) for |: 'type' and 'type'
>>>

Working on a fix.

ColwynGulliford commented 1 month ago

Do you require continued support for Python 3.9? My understanding (from @ken-lauer ) is that lot of libraries follow NEP29 - a deprecation policy that states Python 3.9 is unsupported as of April of this year (On Apr 05, 2024 drop support for Python 3.9 (initially released on Oct 05, 2020)).

MichaelEhrlichman commented 1 month ago

@ChristopherMayes Do we really need to support python 3.9?

ColwynGulliford commented 1 month ago

@ken-lauer put in a fix to make the offending package optional for distgen. Running the same tests with 3.9 now show:

platform darwin -- Python 3.9.20, pytest-8.3.3, pluggy-1.5.0
rootdir: /Users/colwyngulliford/GitHub/distgen
collected 616 items                                                                                                                                                   

distgen/tests/test_beam.py .................................................................................................................................... [ 21%]
ssssssssssssssssssssss......................................................................................................................................... [ 47%]
............................................................................................................................................................... [ 73%]
................................................................................................................................................                [ 96%]
distgen/tests/test_dist.py .....................                                                                                                                [ 99%]
distgen/tests/test_import.py .                                                                                                                                  [100%]

========================================================================== warnings summary ===========================================================================
distgen/physical_constants.py:44
distgen/physical_constants.py:44
  /Users/colwyngulliford/GitHub/distgen/distgen/physical_constants.py:44: ConstantWarning: Constant 'tau mass energy equivalent in MeV' is not in current CODATA 2018 data set
    return scipy.constants.value(key)*unit_registry(scipy.constants.unit(key))

distgen/tests/test_dist.py::test_1d_pdf_from_file
  /Users/colwyngulliford/GitHub/distgen/distgen/dist.py:267: RuntimeWarning: More than 20 figures have been opened. Figures created through the pyplot interface (`matplotlib.pyplot.figure`) are retained until explicitly closed and may consume too much memory. (To control this warning, see the rcParam `figure.max_open_warning`). Consider using `matplotlib.pyplot.close()`.
    plt.figure()

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
============================================================ 594 passed, 22 skipped, 3 warnings in 16.45s =============================================================
(distgen_py39) colwyngulliford@Colwyns-MBP-2 distgen %

I'll make a new release now.