Require Click 7.0+ in Dask

[jakirkham]

Given the recent pain around the move to the new entry points ( dask/distributed#7176 ) and discussion in the fix ( conda-forge/dask-core-feedstock#146 ), would like to propose making Click a hard requirement in Dask.

---

• Closes #xxxx
• Tests added / passed
• Passes pre-commit run --all-files

[douglasdavis]

This looks good to me! Looks like test failures are related to #9597

[jakirkham]

Yeah was looking at that. Looks like it could be related to a recent pytest update

[jakirkham]

Maybe fixed by this pytest-xdist update ( conda-forge/pytest-xdist-feedstock#48 ). Restarting CI...

cc @jrbourbeau (for awareness)

[jakirkham]

Seeing only one CI failure, which doesn't appear related. Raised as issue ( #9598 )

[jakirkham]

CI is passing 🟢

[jrbourbeau]

Thanks @jakirkham!

IIUC the motivation behind this PR is the pain we went through getting the new dask CLI set up on conda-forge. Is that correct, or am I missing something? Since things are now working on conda-forge, I'm wondering how much this change is needed.

To be clear, I don't have a strong objections to adding click -- it's a small, popular, pure Python library. I also suspect many (most?) Dask users today already install click because pip install dask[complete] and conda install dask pull in click. I'm just trying to get a better understanding for what problem gets solved by adding click as a dependency.

cc @jacobtomlinson as I believe he had some reservations around making click a required dependency (xref #9283 (comment))

[jacobtomlinson]

My reservations were around insisting folks use click to implement the downstream plugins. Tools like dask-yarn use argparse so can't be integrated with the new CLI unless they migrate to click. However, I conceded on this point as supporting argparse was too technically difficult.

I'm not against this change 😃

[jakirkham]

It seems the mindeps changes are causing CI issues. Are we attached to those changes @jrbourbeau?

[jrbourbeau]

I'd suggest we keep the tight pin and update the minimum version of click to something more recent that can be solved. Does click=7.1.2 work? We should be able to install everything when pinned to the earliest supported version.

[jakirkham]

Sure we can give that a try. Just curious why 7.1.2 was chosen?

[jrbourbeau]

It's much newer than 6.6 (so hopefully will solve -- though I've not confirmed this) but also seemed old enough to (hopefully) not be problematic for users to install into their environments. But yeah, this was definitely just an educated guess

[jakirkham]

Gotcha

Took a deeper look and think the issue is that click 6.6 (and 6.7 for that matter) were not noarch. Plus the Python versions supported at the time were quite a bit older (the newest Python version built with was 3.6 for perspective). As a result we can't install these with the Python versions we use here.

The first version of click to be noarch was 7.0. So going to try that. Admittedly there may still be issue with that version of click (like dependency conflicts), but hopefully we will get more info from the solver to inform the next version choice (if needed).

[jakirkham]

Looks like CI was cancelled?

In any event it appears the installs in mindeps worked.

[jrbourbeau]

🎉 glad the install is working now. Though it looks like there are now related test failures

___________________________ test_register_command_ep ___________________________

    def test_register_command_ep():
        from dask.cli import _register_command_ep
    
        bad_ep = importlib.metadata.EntryPoint(
            name="bad",
            value="dask.tests.test_cli:bad_command",
            group="dask_cli",
        )
    
        good_ep = importlib.metadata.EntryPoint(
            name="good",
            value="dask.tests.test_cli:good_command",
            group="dask_cli",
        )
    
        with pytest.warns(UserWarning, match="must be instances of"):
            _register_command_ep(dummy_cli, bad_ep)
    
>       _register_command_ep(dummy_cli, good_ep)

dask/tests/test_cli.py:80: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

interface = <function command.<locals>.decorator at 0x7fc89050b1f0>
entry_point = EntryPoint(name='good', value='dask.tests.test_cli:good_command', group='dask_cli')

    def _register_command_ep(interface, entry_point):
        """Add `entry_point` command to `interface`.
    
        Parameters
        ----------
        interface : click.Command or click.Group
            The click interface to augment with `entry_point`.
        entry_point : importlib.metadata.EntryPoint
            The entry point which loads to a ``click.Command`` or
            ``click.Group`` instance to be added as a sub-command or
            sub-group in `interface`.
    
        """
        command = entry_point.load()
        if not isinstance(command, (click.Command, click.Group)):
            warnings.warn(
                "entry points in 'dask_cli' must be instances of "
                f"click.Command or click.Group, not {type(command)}."
            )
            return
    
>       if command.name in interface.commands:
E       AttributeError: 'function' object has no attribute 'commands'

dask/cli.py:64: AttributeError

[douglasdavis]

interface in that code should be an instance of click.Group. I wonder why it's coming out to just be a function object in mindeps but is what is expected in the other tests.. I'm still digging. Or maybe there's another way we can check and warn for repeated command names.

[jrbourbeau]

Thanks for looking into it @douglasdavis

[douglasdavis]

Ah it's because my "dummy" CLI's are bad in the test file :) (Click pre v8 requires parens on the decorators)

necessary fix:

diff --git a/dask/tests/test_cli.py b/dask/tests/test_cli.py
index a7e2d5f86..2adab2b77 100644
--- a/dask/tests/test_cli.py
+++ b/dask/tests/test_cli.py
@@ -40,7 +40,7 @@ def test_info_versions():
     assert table["distributed"] == distributed_version
 
 
-@click.group
+@click.group()
 def dummy_cli():
     pass
 
@@ -82,7 +82,7 @@ def test_register_command_ep():
     assert dummy_cli.commands["good"] is good_command
 
 
-@click.group
+@click.group()
 def dummy_cli_2():
     pass
 

[jakirkham]

Awesome thanks Doug and James! 🙏

[jakirkham]

Should we bump the click requirement to 7.0 in setup.py & meta.yaml too?

[jrbourbeau]

I'm just trying to get a better understanding for what problem gets solved by adding click as a dependency.

Circling back to this point, one thing I'd like to do is update our issue template to use dask info versions instead of asking separately for dask, distributed, and python versions as running dask info versions is less of an ask for users. Having click installed already will reduce barriers for this. I'll admit I only see this as moderate motivation for adding click as a dependency, but I also think that's probably okay in this case.

I'll propose we give a bit more time for others to weigh in (@jakirkham asked for feedback here dask/community#283 (comment) a few days ago) and then merge prior to releasing today.

[jakirkham]

Thanks James! 🙏

[jakirkham]

Given we are using Click 7.0+ in Dask, thought it would make sense to align on that minimum version in Distributed. Submitted PR ( dask/distributed#7226 ) to do that. Feedback welcome :)