ocelot-collab / ocelot

OCELOT is a multiphysics simulation toolkit designed for studying FEL and storage ring-based light sources.
GNU General Public License v3.0
84 stars 56 forks source link

with old Python: instead of message informing about min required version, OCELOT drops long backtrace #234

Closed ZeugAusHH closed 2 months ago

ZeugAusHH commented 4 months ago

Running OCELOT with very old Python versions gives a long backtrace instead of user friendly message "OCELOT requires Python 3.x or higher".

I tested this with v3.6.8 (which dates back to 2018) and is the default on MAXWELL cluster unless you choose to activate different version by loading modules. The test was done with the current status of OCELOT master branch, 18f64f22baa9983cc5e2e259adb44ffa9e08fa6b.

Probably a test for the minimum requirement should be added to https://github.com/ocelot-collab/ocelot/blob/master/ocelot/__init__.py .

demo script

[lechnerc@max-display008 20240424__ocelot_version]$ cat 20240424__ocelot_version.py
#!/usr/bin/env python3

# C. Lechner, EuXFEL, 2024-04-24

import sys
print('running '+sys.version)

# With old Python version this drops long backtrace.
# A message "OCELOT requires Python 3.x or higher" would be nicer...
import ocelot

from ocelot.utils.git_commit_id import get_git_commit_id
print('using OCELOT source code with git commit id: '+get_git_commit_id())

[lechnerc@max-display008 20240424__ocelot_version]$

With Python v3.9.16

[lechnerc@max-display008 20240424__ocelot_version]$ module list
Currently Loaded Modulefiles:
 1) maxwell   2) mamba/3.9
[lechnerc@max-display008 20240424__ocelot_version]$ ./20240424__ocelot_version.py
running 3.9.16 | packaged by conda-forge | (main, Feb  1 2023, 21:39:03)
[GCC 11.3.0]
[INFO    ] csr.py: module PYFFTW is not installed. Install it to speed up calculation.
initializing ocelot...
import: module PYFFTW is not installed. Install it to speed up calculation
using OCELOT source code with git commit id: 18f64f22baa9983cc5e2e259adb44ffa9e08fa6b

Now with very old Python version 3.6.8

[lechnerc@max-display008 20240424__ocelot_version]$ module purge
[lechnerc@max-display008 20240424__ocelot_version]$ module list
No Modulefiles Currently Loaded.
[lechnerc@max-display008 20240424__ocelot_version]$ ./20240424__ocelot_version.py
running 3.6.8 (default, Nov 14 2023, 16:29:52)
[GCC 4.8.5 20150623 (Red Hat 4.8.5-44)]
Traceback (most recent call last):
  File "./20240424__ocelot_version.py", line 10, in <module>
    import ocelot
  File "/[..]/ocelot/__init__.py", line 47, in <module>
    from ocelot.cpbd.magnetic_lattice import MagneticLattice, merger
  File "/[..]/ocelot/cpbd/magnetic_lattice.py", line 1, in <module>
    from ocelot.cpbd.elements.optic_element import OpticElement
  File "/[..]/ocelot/cpbd/elements/__init__.py", line 6, in <module>
    from ocelot.cpbd.elements.aperture import Aperture
  File "/[..]/ocelot/cpbd/elements/aperture.py", line 3, in <module>
    from ocelot.cpbd.elements.optic_element import OpticElement
  File "/[..]/ocelot/cpbd/elements/optic_element.py", line 2, in <module>
    from ocelot.cpbd.transformations.transfer_map import TransferMap
  File "/[..]/ocelot/cpbd/transformations/__init__.py", line 4, in <module>
    from ocelot.cpbd.transformations.cavity import CavityTM
  File "/[..]/ocelot/cpbd/transformations/cavity.py", line 3, in <module>
    from ocelot.cpbd.transformations.transfer_map import TransferMap, TMTypes
  File "/[..]/ocelot/cpbd/transformations/transfer_map.py", line 5, in <module>
    from ocelot.cpbd.transformations.transformation import Transformation, TMTypes
  File "/[..]/ocelot/cpbd/transformations/transformation.py", line 8, in <module>
    from ocelot.cpbd.beam import Particle, ParticleArray
  File "/[..]/ocelot/cpbd/beam.py", line 1670, in <module>
    class SliceParameters:
  File "/[..]/ocelot/cpbd/beam.py", line 1685, in SliceParameters
    "gamma_y": "gamma_y"}
TypeError: 'type' object is not subscriptable
[lechnerc@max-display008 20240424__ocelot_version]$
st-walker commented 4 months ago

If we don't support Python 3.6 (which I don't think we should, because it is ancient) then the solution is not to have a nice error when imported, but rather to not allow it to be installed in the first place. If somehow a user manages to install OCELOT on an ancient, unsupported version of Python (e.g. 2.7) and they have a nasty traceback, that will then be their problem.

OCELOT is installed using setuptools. We just need to add the right kwarg python_requires to the setup function:

https://packaging.python.org/en/latest/guides/dropping-older-python-versions/#specify-the-version-ranges-for-supported-python-distributions

Who wants to do it? Is it even really clear which Python versions we support exactly? @sergey-tomin is the expert here.

ZeugAusHH commented 4 months ago

I agree 100% that Python 3.6 shouldn't be supported. Still, I do think that even if somebody clones from github, they should still get a nice message instead of the traceback. In the end this is just a simple if like all the other checks performed __init__.py.

I never implemented such version checking in setuptools, so I wouldn't implement it.

st-walker commented 4 months ago

I agree that it is a mistake that we have no way of preventing bad/old Pythons from being used. I prefer the python_requires as it adds to the already mostly declarative set of requirements in the setup.py rather than moving them into the code, which then needs to be remembered if we ever deprecate 3.7 (I think we should already have dumped anything older than 3.9 personally because numpy, matplotlib scipy all either require 3.9 or 3.10 at least, and we are not special).

To be honest we should probably bin the setup.py altogether and move to pyproject.toml.

I don't mind implementing something like this but I suppose it would be nice to understand what minor versions of Python we are actually going to support and which to bin.

ZeugAusHH commented 4 months ago

Concerning the minimum Python versions required by the different parts of OCELOT: Different parts may very well have different requirements. Probably @sergey-tomin and @sserkez know better.

But what about creating the "infrastructure" for it and just block out very old stuff such as the 3.6.x versions to begin with?

st-walker commented 3 months ago

In principle yes it's a nice idea but if the users need to pull from git and reinstall ocelot to benefit from this warning that their Python version is too old, then we might as well just cut to the chase and prevent them from installing in the first place.

st-walker commented 3 months ago

We need @sergey-tomin input to proceed with this, which Python versions we say we support and which we get rid of. I am not sure if we are even testing against all versions of Python we supposedly support.

ZeugAusHH commented 3 months ago

Assuming that it is not very much work, I would propose that we already build the framework for the Python version check. Users running 3.6.x could be stopped already now because it just doesn't work for 3.6.x (actually improving the situation for them because they would see a "nice" error message instead of that backtrace). And once we know what is the oldest officially supported version we adjust the version numbers...

By the way, I typically use OCELOT from a cloned directory that I keep up-to-date as needed. This enables me to quickly jump around between branches etc. So in my use case this version check would definitely be great.

st-walker commented 3 months ago

We have no hard definition of which Python OCLEOT even supports as far as I am aware, so first we would need that. Even if we did, I am opposed to this, I think it is not appropriate for OCELOT to start checking Python versions. I just checked numpy and they don't check for python version as far as I can tell. Llvmlite does the same, they do it at install time like I suggested originally.

By the way, I typically use OCELOT from a cloned directory that I keep up-to-date as needed

Me too. If we have minimum version in setup.py or pyproject.toml then we are fine. Beyond that, I don't even know how one would manage to mix python versions with ocelot, unless they are abusing PYTHONPATH, which is their problem and one should generally not be doing anyway.

st-walker commented 3 months ago

Hey @sergey-tomin and @ZeugAusHH i found this:

https://devguide.python.org/versions/#

versions 3.6 and 3.7 are now completely unsupported by Python. We shouldn't be using them nor supporting them, if not even Python itself uses/supports them. 3.8 will be dumped October this year. Let's get with the times and embrace the future together as one strong ocelot family.

st-walker commented 3 months ago

@sergey-tomin asked me to look into this so I did.

I tried installing ocelot starting with python3.6 and for each version check if ocelot imports and so on.

3.6

Doesn't work. Things this version hates about ocelot:

stopped editing ocelot at this point to find the next error, it doesn't work, move on.

3.7

i deleted these lines and all tests passed

3.8

Doesn't even install on my m1 macbook. I suspect this is a problem i have with my python environment though rather than an ocelot problem, but i don't care at all about fixing this to find out more. the error:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/stuartwalker/repos/ocelot/ocelot/__init__.py", line 47, in <module>
    from ocelot.cpbd.magnetic_lattice import MagneticLattice, merger
  File "/Users/stuartwalker/repos/ocelot/ocelot/cpbd/magnetic_lattice.py", line 1, in <module>
    from ocelot.cpbd.elements.optic_element import OpticElement
  File "/Users/stuartwalker/repos/ocelot/ocelot/cpbd/elements/__init__.py", line 6, in <module>
    from ocelot.cpbd.elements.aperture import Aperture
  File "/Users/stuartwalker/repos/ocelot/ocelot/cpbd/elements/aperture.py", line 3, in <module>
    from ocelot.cpbd.elements.optic_element import OpticElement
  File "/Users/stuartwalker/repos/ocelot/ocelot/cpbd/elements/optic_element.py", line 2, in <module>
    from ocelot.cpbd.transformations.transfer_map import TransferMap
  File "/Users/stuartwalker/repos/ocelot/ocelot/cpbd/transformations/__init__.py", line 4, in <module>
    from ocelot.cpbd.transformations.cavity import CavityTM
  File "/Users/stuartwalker/repos/ocelot/ocelot/cpbd/transformations/cavity.py", line 3, in <module>
    from ocelot.cpbd.transformations.transfer_map import TransferMap, TMTypes
  File "/Users/stuartwalker/repos/ocelot/ocelot/cpbd/transformations/transfer_map.py", line 5, in <module>
    from ocelot.cpbd.transformations.transformation import Transformation, TMTypes
  File "/Users/stuartwalker/repos/ocelot/ocelot/cpbd/transformations/transformation.py", line 8, in <module>
    from ocelot.cpbd.beam import Particle, ParticleArray
  File "/Users/stuartwalker/repos/ocelot/ocelot/cpbd/beam.py", line 15, in <module>
    from ocelot.common.math_op import find_nearest_idx
  File "/Users/stuartwalker/repos/ocelot/ocelot/common/math_op.py", line 12, in <module>
    import numba as nb
  File "/Users/stuartwalker/Library/Python/3.8/lib/python/site-packages/numba/__init__.py", line 211, in <module>
    import numba.typed
  File "/Users/stuartwalker/Library/Python/3.8/lib/python/site-packages/numba/typed/__init__.py", line 1, in <module>
    from .typeddict import Dict
  File "/Users/stuartwalker/Library/Python/3.8/lib/python/site-packages/numba/typed/typeddict.py", line 23, in <module>
    def _make_dict(keyty, valty):
  File "/Users/stuartwalker/Library/Python/3.8/lib/python/site-packages/numba/core/decorators.py", line 260, in njit
    return jit(*args, **kws)
  File "/Users/stuartwalker/Library/Python/3.8/lib/python/site-packages/numba/core/decorators.py", line 183, in jit
    return wrapper(pyfunc)
  File "/Users/stuartwalker/Library/Python/3.8/lib/python/site-packages/numba/core/decorators.py", line 210, in wrapper
    disp = dispatcher(py_func=func, locals=locals,
  File "/Users/stuartwalker/Library/Python/3.8/lib/python/site-packages/numba/core/dispatcher.py", line 777, in __init__
    self.targetctx = self.targetdescr.target_context
  File "/Users/stuartwalker/Library/Python/3.8/lib/python/site-packages/numba/core/registry.py", line 47, in target_context
    return self._toplevel_target_context
  File "/Users/stuartwalker/Library/Python/3.8/lib/python/site-packages/numba/core/utils.py", line 277, in __get__
    res = instance.__dict__[self.name] = self.func(instance)
  File "/Users/stuartwalker/Library/Python/3.8/lib/python/site-packages/numba/core/registry.py", line 31, in _toplevel_target_context
    return cpu.CPUContext(self.typing_context)
  File "/Users/stuartwalker/Library/Python/3.8/lib/python/site-packages/numba/core/base.py", line 260, in __init__
    self.init()
  File "/Users/stuartwalker/Library/Python/3.8/lib/python/site-packages/numba/core/compiler_lock.py", line 35, in _acquire_compile_lock
    return func(*args, **kwargs)
  File "/Users/stuartwalker/Library/Python/3.8/lib/python/site-packages/numba/core/cpu.py", line 47, in init
    self._internal_codegen = codegen.JITCPUCodegen("numba.exec")
  File "/Users/stuartwalker/Library/Python/3.8/lib/python/site-packages/numba/core/codegen.py", line 1100, in __init__
    self._init(self._llvm_module)
  File "/Users/stuartwalker/Library/Python/3.8/lib/python/site-packages/numba/core/codegen.py", line 1105, in _init
    target = ll.Target.from_triple(ll.get_process_triple())
  File "/Users/stuartwalker/Library/Python/3.8/lib/python/site-packages/llvmlite/binding/targets.py", line 197, in from_triple
    raise RuntimeError(str(outerr))
RuntimeError: No available targets are compatible with triple "aarch64-apple-darwin23.2.0"

3.9

imports fine. didn't run tests. assume they pass since they passed in 3.7.

3.10+

Didn't check these. Python 3.12 works fine.

so there you go.

in any case i oppose adding code at import time to check for python 3.6, if you run python 3.6 this is a problem you have, e.g. also if you run ruby interpreter on ocelot code or if you try to compile it as c++.

sergey-tomin commented 3 months ago

Thank you @st-walker So, what's your proposal? Do we support Python 3.8+ or...? Regarding the traceback or nice message, I think highlighting the line with the supported Python version in README should be sufficient.

st-walker commented 3 months ago

bin 3.7 and 3.6. support 3.8 but in time we should bin 3.8 too. 3.9+ we carry on with.

Regarding the traceback or nice message, I think highlighting the line with the supported Python version in README should be sufficient.

yes it is not ocelot's job to help you if you use python 2 or something. it's not our problem. I propose we just update the README and close this issue.

SchroederSa commented 2 months ago

I just ran into the same problem. Python 3.9 is the first version that worked fine for me. I had an issue with 9.11, but I can't remember the error message anymore.

Could the Python version suggestion be changed in the README?

The requirement 'Python 3.6 - 3.8' is very misleading.

Thanks! Sarah

st-walker commented 2 months ago

Hi, I made pull request here: #254

readme.md is updated to tell the truth now, and also it won't even install for older versions of Python, telling you it is too low a version.

I agree it is confusing. We should not support such ancient versions of Python, let alone suggest we only support such ancient versions of Python (when we actually do not!).

st-walker commented 2 months ago

I'm closing this as we have a solution, we don't let the user install for python versions < 3.9.