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

Drop OSX builds for python 3.9 #6073

Merged
merged 3 commits into from
Apr 8, 2022
Merged

Drop OSX builds for python 3.9 #6073

merged 3 commits into from
Apr 8, 2022

Conversation

fjetter
Copy link
Member

@fjetter fjetter commented Apr 5, 2022

Closes #6040

I went a bit further than what has been discussed in the above issue, mostly to have things cleaned up a bit.

  • I cleaned up the 3.9 build since I believe we should have one build with as minimal dependencies as possible (e.g. to verify that we don't have obscure import errors)
  • We need to test all of our optional dependencies, this is now done in 3.9
  • Lastly, we want to test against upstream changes for some of our important dependencies where we're testing against git tips

Based on this cleanup, I figured the most important build to keep around for OSX would be the "all dependencies" one which is 3.9 atm.

If this cleanup is too aggressive, we can go with Matt's proposal and just drop py3.9

@github-actions
Copy link
Contributor

github-actions bot commented Apr 5, 2022

Unit Test Results

       16 files   -        2         16 suites   - 2   7h 27m 15s ⏱️ - 1h 26m 10s
  2 724 tests ±       0    2 643 ✔️ +       3       81 💤  -     1  0  - 2 
21 665 runs   - 2 699  20 577 ✔️  - 2 569  1 088 💤  - 128  0  - 2 

Results for commit 41d003a. ± Comparison against base commit 971a96d.

♻️ This comment has been updated with latest results.

@crusaderky
Copy link
Collaborator

I don't think this is wise. You're skipping all code specific to a Python version that test an optional dependency.

Artificial, but realistic example:

mymod.py:

from __future__ import annotations
from typing import cast

def f(x: int | dict[str, int]):
    import scipy
    if some_condition:  # anything but isinstance()
        return scipy.foo(cast(int, x))
    else:
        return scipy.bar(cast(dict[str, int], x))

test_mymod.py:

def test_f():
    pytest.import_or_skip("scipy")
    assert f(1) == ...
    assert f({"x": 1}) == ...

The above fails on Python 3.8 because of the cast(dict[str, int], x) without quotes; however the test suite won't catch it anymore!

@fjetter
Copy link
Member Author

fjetter commented Apr 7, 2022

Do you suggest to use the minimal python version to test all optional dependencies? I believe there is always a trade off unless we always test everything.

We can postpone this decision until after the monthly dev meeting later today. The agenda is proposing to drop cython which would also have an impact on our test matrix.

@crusaderky
Copy link
Collaborator

Currently we have:

Python version OS Which deps deps version
3.8 Linux, MacOSX, Windows mandatory, optional, Cython latest
3.9 Linux, MacOSX, Windows mandatory, optional, CUDA latest
3.10 Linux, MacOSX, Windows mandatory, optional latest+git tip

I think we should change it to:

Python version OS Which deps deps version
3.8 Linux mandatory, optional, CUDA pinned to minimum
3.8 Linux mandatory only (no numpy!) latest
3.8 Linux, MacOSX, Windows mandatory, optional, CUDA latest
3.9 Linux mandatory, optional, CUDA latest
3.10 Linux, MacOSX, Windows mandatory, optional, CUDA latest+git tip

That's 9 workflows before and after, but with one less Mac workflow and a lot of added value.
I expect the first two workflows to require a lot of effort before they become green.

@fjetter
Copy link
Member Author

fjetter commented Apr 7, 2022

That's 9 workflows before and after, but with one less Mac workflow and a lot of added value.
I expect the first two workflows to require a lot of effort before they become green.

I'm open to setting it up this way but that requires much more effort than what I'm willing to put in this at this point in time.

What I wanted to rectify here is that we have "partial optional" all over the place but if there is more demand for a conversation, how about the following

Python version OS Which deps deps version
3.8 Linux, MacOSX, Windows mandatory, optional-as-is, Cython, CUDA latest
3.9 Linux, Windows mandatory, optional-as-is latest
3.10 Linux, Windows mandatory, optional-as-is latest+git tip

We have a few only-tested-here in the py3.9 build but I don't think there is a lot of sensitivity to OSX in there

@crusaderky
Copy link
Collaborator

What's "partial optional" / optional-as-is? I see

3.8 only

  • cython
  • crick (depends on cython)

3.9 only

  • pynvml (CUDA)
  • pytorch (CUDA)
  • torchvision (CUDA)
  • lz4
  • python-blosc
  • python-snappy

I would not disable MacOS testing on 3.10 and I would not skip the compression libs either.

As a transitory low-effort matrix I'd suggest

Python version OS Which deps deps version
3.8 Linux reduced (none from the above list); also remove those already tested as removable in this PR latest
3.8 Linux, MacOSX, Windows mandatory, optional, CUDA, Cython, crick latest
3.9 Linux mandatory, optional, CUDA latest
3.10 Linux, MacOSX, Windows mandatory, optional, CUDA latest+git tip

@fjetter
Copy link
Member Author

fjetter commented Apr 7, 2022

I would not disable MacOS testing on 3.10 and I would not skip the compression libs either.

I do consider the compression libs and a few other things like scipy optional. Essentially everything that's currently not in requirements.txt

However, my suggestion in #6073 (comment) meant simply to move pynvml to py3.8 and drop the py3.9 build

I would not disable MacOS testing on 3.10

why?

@crusaderky
Copy link
Collaborator

I would not disable MacOS testing on 3.10

why?

In 3.10 specifically, nothing comes to mind. But there have been cases in the past for a major OS-specific change. For example, in 3.8 the default mp-context changed from fork to spawn on MacOSX. It would be very bad practice to skip testing it just because nothing catches our eyes from the release notes.

@fjetter
Copy link
Member Author

fjetter commented Apr 7, 2022

In 3.10 specifically, nothing comes to mind. But there have been cases in the past for a major OS-specific change. For example, in 3.8 the default mp-context changed from fork to spawn on MacOSX. It would be very bad practice to skip testing it just because nothing catches our eyes from the release notes.

My argument is mostly that the cost is very high to keep this up and the likelihood for an error very small.

@fjetter
Copy link
Member Author

fjetter commented Apr 7, 2022

Unrelated question: Do we need CUDA tests everywhere? Isn't this covered by the gpuCI?

@fjetter
Copy link
Member Author

fjetter commented Apr 7, 2022

The current state is minimal and just drops py3.9 and moves pynvml to py3.8.

We can discuss further changes to the test matrix in a separate issue. I think this would already help a lot with the bottleneck and I would like to get this in fast

@fjetter
Copy link
Member Author

fjetter commented Apr 7, 2022

I opened #6085 for any follow up conversations

@@ -18,15 +18,13 @@ dependencies:
- ipykernel
- ipywidgets
- jinja2
- joblib
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last reference to this was removed in #3040

@fjetter fjetter changed the title Drop OSX builds for 3.8 and 3.10 Drop OSX builds for python 3.9 Apr 8, 2022
@fjetter fjetter merged commit 0e0472f into dask:main Apr 8, 2022
@fjetter fjetter deleted the drop_osx_builds branch April 8, 2022 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drop Mac Python 3.9 builds?
2 participants