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

Extend support to Python 3.11 #258

Merged
merged 12 commits into from
May 11, 2023
Merged

Extend support to Python 3.11 #258

merged 12 commits into from
May 11, 2023

Conversation

alecandido
Copy link
Member

@alecandido
Copy link
Member Author

c309a03 is failing on py3.8 and py3.10, but not on py3.9...

There is some non-deterministic bug coming with some of the upgraded deps...

@alecandido alecandido requested a review from felixhekhorn May 3, 2023 16:18
@alecandido alecandido removed the request for review from felixhekhorn May 3, 2023 16:18
@alecandido alecandido self-assigned this May 3, 2023
@felixhekhorn felixhekhorn added the dependencies Pull requests that update a dependency file label May 4, 2023
@felixhekhorn
Copy link
Contributor

There seems to be two different type of errors around:

  1. a numerical issue present in all Python versions 3.8 - 3.1:
FAILED tests/eko/test_interpolation.py::TestBasisFunction::test_log_eval_N - AssertionError: 
Arrays are not almost equal to 7 decimals
 ACTUAL: 0.718281828459045
 DESIRED: 2.718281828459045

this might be related to #241
2. an IO issue present only in 3.8

/home/runner/work/eko/eko/src/eko/io/dictlike.py:192: in load_typing
    return load_field(variant, value)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

type_ = numpy.ndarray[typing.Any, numpy.dtype[+ScalarType]], value = None

    def load_field(type_, value):
        """Deserialize dataclass field."""
        # TODO: nice place for a match statement...
        if type(type_) is typing.NewType:
            return load_field(type_.__supertype__, value)
        try:
            # before py3.10 typing.NewType was just a function, so the check above
            # would  fail
            return load_field(type_.__supertype__, value)
        except AttributeError:
            pass
    
        if typing.get_origin(type_) is not None:
            # this has to go first since for followin ones I will assume they are
            # valid classes, cf. the module docstring
            return load_typing(type_, value)
    
>       assert inspect.isclass(type_)
E       AssertionError

@alecandido this looks like numpy has changed something

@felixhekhorn
Copy link
Contributor

On top there is a new numba issue inside OME appearing (sounds like something is not an array, but it should be ...)

@giacomomagni
Copy link
Collaborator

On top there is a new numba issue inside OME appearing (sounds like something is not an array, but it should be ...)

Maybe it's because A is defined only inside if and thus can be None ?

@felixhekhorn
Copy link
Contributor

On top there is a new numba issue inside OME appearing (sounds like something is not an array, but it should be ...)

Maybe it's because A is defined only inside if and thus can be None ?

I believe you're correct - the solution has been 49f2078

PS: I guess another hint we should move away from numba ...

@alecandido
Copy link
Member Author

alecandido commented May 10, 2023

The other problem is that some of the updated deps is now causing issues with typing in py3.8...

Since it is limited to "new dep + py3.8", I'd be really tempted to say: well, let's drop py3.8. However, that is definitely not an option, it is still incredibly popular (see "Python Minor" download plots for NumPy and all packages on PyPI).
To be fair, even py3.7 is still very used, actually the most used one: but that's their fault, it is really approaching EOL...

EDIT: in particular, the offending dependency is NumPy, not Numba, so this is a separate problem that has to be solved anyhow

@felixhekhorn
Copy link
Contributor

Okay, problem 1 is solved: this was due to float numerics, i.e. np.log and np.exp are not inverse functions to each other. This fact we knew all along, but still so far it did work - I assume they might have been on a subset of float numbers. Apparently now one of the two changed its implementation and now they diverge also for our test case. Solution was to relax the comparisons in log_evaluate_Nx - should we adjust also the others? (evaluate_Nx and evaluate_x with the later being adjusted already halfway)

@giacomomagni
Copy link
Collaborator

giacomomagni commented May 10, 2023

np.log and np.exp are not inverse functions

Just for curiosity where did you see it? https://numpy.org/doc/stable/reference/generated/numpy.log.html

@felixhekhorn
Copy link
Contributor

np.log and np.exp are not inverse functions

Just for curiosity where did you see it? https://numpy.org/doc/stable/reference/generated/numpy.log.html ?

I deduced from here https://github.com/NNPDF/eko/actions/runs/4936234492/jobs/8823523488#step:11:124 (where a np.log(np.exp(-1)) call took place) - and it makes sense, I think ...

@giacomomagni
Copy link
Collaborator

I deduced from here https://github.com/NNPDF/eko/actions/runs/4936234492/jobs/8823523488#step:11:124 (where a np.log(np.exp(-1)) call took place) - and it makes sense, I think ...

I see then the definition in the documentation is not fully accurate 😬

@alecandido
Copy link
Member Author

I see then the definition in the documentation is not fully accurate 😬

I believe the definition to be accurate enough, simply is defined something different of what you think: they are defining the $log(x)$ function, as a function on real numbers, for the user who might wonder what the np.log(x) is implementing (or, if you wish, approximating). E.g. in case you had doubts on the basis.

However, the np.log(x) is a function float -> float, and not $\mathbb{R} \to \mathbb{R}$, and as a float function is not the inverse of np.exp(x).

@alecandido
Copy link
Member Author

The remaining issue arises from npt.NDArray: until numpy==1.23.1 it was a class, so there were two different behaviors between py3.8 and py>3.8, but both valid

  • py>3.8 is able to find the origin class of the type hint

    eko/src/eko/io/dictlike.py

    Lines 144 to 147 in 999fb30

    if typing.get_origin(type_) is not None:
    # this has to go first since for followin ones I will assume they are
    # valid classes, cf. the module docstring
    return load_typing(type_, value)

    once resolved, that class (i.e. np.ndarray) is immediately used
  • py3.8 was not able to reconstruct the origin, but since npt.NDArray was a class on its own, it was passing this gate
    assert inspect.isclass(type_)

    and resolved later on as NumPy array

    eko/src/eko/io/dictlike.py

    Lines 155 to 159 in 999fb30

    if issubclass(type_, np.ndarray) or np.ndarray in type_.__mro__:
    # do not apply array on scalars
    if isinstance(value, list):
    return np.array(value)
    return value

However, since numpy==1.23.2, npt.NDArray is not any longer a class, and for this reason py3.8 is failing.
From the list of PRs merged, I believe the responsible to be numpy/numpy#22032.

However, the weird behavior is first an issue in py3.8, for which I do not understand why typing.get_origin is behaving differently than in the other Python versions (the function has been introduced in py3.8 itself, and no public change is reported in the docs).

@alecandido
Copy link
Member Author

My change is minimal (just sorting differently some conditions that were already present), and seems like it is doing its job.

Even @felixhekhorn changes are minimal, and perfectly reasonable (I would approve them with the tick, but I'm the author of the PR).

Let's wait for the CI, and if @giacomomagni approves them as well I would merge immediately.

Btw, I just stepped into py3.11 on my system and I'd like to be able to run EKO :P
(@felixhekhorn is even waiting since longer)

@alecandido
Copy link
Member Author

Thanks @giacomomagni I'm just waiting a little more to give @felixhekhorn the chance to have a final look.

I will merge today in any case.

@alecandido alecandido merged commit fe0614a into master May 11, 2023
@alecandido alecandido deleted the python3.11 branch May 11, 2023 13:19
@alecandido alecandido mentioned this pull request May 11, 2023
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants