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

Fix compatibility with distributed >= 2022.5.1 and traitlets >= 5.2.0, and raise the lower bound of required versions #573

Merged
merged 4 commits into from
Jun 13, 2022

Conversation

consideRatio
Copy link
Collaborator

@consideRatio consideRatio commented Jun 10, 2022

Closes #571.

I've observed a flurry of issues that created a very hard to debug situation, but this resolves them all I think. This patch makes us require modern versions of distributed, traitlets, click, and dask though. Unless we have tests against the minimum required versions, I think its better that we do this than hope a version could be slightly lower without issues though.


The click issue as seen on a scheduler pod starting and quickly terminating.

Traceback (most recent call last):
  File "/srv/conda/envs/notebook/bin/dask-scheduler", line 8, in <module>
    sys.exit(go())
  File "/srv/conda/envs/notebook/lib/python3.9/site-packages/distributed/cli/dask_scheduler.py", line 216, in go
    check_python_3()
  File "/srv/conda/envs/notebook/lib/python3.9/site-packages/distributed/cli/utils.py", line 39, in check_python_3
    from click import _unicodefun
ImportError: cannot import name '_unicodefun' from 'click' (/srv/conda/envs/notebook/lib/python3.9/site-packages/click/__init__.py)

@consideRatio consideRatio force-pushed the pr/avoid-click-failure branch from 38396a5 to 5cdfc78 Compare June 10, 2022 07:02
@consideRatio
Copy link
Collaborator Author

When we don't have distributed pinned below 2022.5.0, we get:

image

When we don't have traitlets pinned below 5.2.0, we get:

image

@consideRatio consideRatio changed the title debugging: trying to resolve critical failure related to click debugging: trying to resolve critical failure related to traitlets/distributed/click? Jun 10, 2022
@consideRatio
Copy link
Collaborator Author

It seems that in distributed 2022.05.1, dask/distributed#6363 was introduced, and that removes a fast parameter from scheduler.close(). So, dask-gateway must use <=2022.05.0 and we should require >=2022.05.1 after we adjust to this removal.

@consideRatio
Copy link
Collaborator Author

consideRatio commented Jun 10, 2022

Traitlets 5.2.0

image

image

Traitlets 5.2.1

Broken in a way specific to 5.2.1, fixed in 5.2.2.

image

Traitlets 5.2.2.post.1

Same as 5.2.0.

@consideRatio consideRatio force-pushed the pr/avoid-click-failure branch from b295db8 to eb49e86 Compare June 10, 2022 12:18
@consideRatio consideRatio force-pushed the pr/avoid-click-failure branch from eb49e86 to 8eee259 Compare June 10, 2022 12:23
@consideRatio consideRatio changed the title debugging: trying to resolve critical failure related to traitlets/distributed/click? Fix for compatibility with distributed >= 2022.5.1 and traitlets >= 5.2.0 Jun 10, 2022
@consideRatio consideRatio changed the title Fix for compatibility with distributed >= 2022.5.1 and traitlets >= 5.2.0 Fix compatibility with distributed >= 2022.5.1, traitlets >= 5.2.0, and click >= 8.1.3 and require it Jun 10, 2022
@consideRatio consideRatio added the bug Something isn't working label Jun 10, 2022
@TomAugspurger
Copy link
Member

Haven't looked closely here, but linking to dask/distributed#6561. With that PR to distributed, we can perhaps avoid changing the dask / distributed requirements. The current release will still be incompatible distributed=2022.5.2, but the current release will be compatible with older and newer versions (with these changes, it'll be compatible with those versions plus distributed=2022.5.2)

@consideRatio
Copy link
Collaborator Author

consideRatio commented Jun 10, 2022

EDIT: Compromise suggestion in #573 (review). My concern relates to #539.


It adds maintenance burden to ensure that things work with old versions, so keeping an old lower bound that we don't test doesn't feel great.

What should be done ideally, is to run tests against the oldest versions declared support for, like done in jupyterhub: https://github.com/jupyterhub/jupyterhub/blob/f6eec29aa222276ea8a9d8066eabb53e9f65b1a7/.github/workflows/test.yml#L132-L159. If that is done, it seems more acceptable to me to retain an old lower version bound. But of course, this adds maintenance burden - especially if we add such tests with very old lower bounds that are around for historical reasons.

I have worked very hard to reduce the complexity of this project's many separate dependencies, reducing complexity has been critical for the maintenance sustainability. Due to that it feels troublesome to add in >=X,!=Y,!=Z where X is very old and untested. This project has so many sets of different extra_requires for the packages, a client side, server side, and docker images with dependencies getting fixed during build - I think now is a good time time to bump X to a new version.

If you don't think retaining support for a broad version range is quite important, I suggest we still make dask-gateway require a modern version of these dependencies to reduce our maintenance burden and help users end up with something more likely to work, even if it means they are forced to make some updates.

@consideRatio consideRatio mentioned this pull request Jun 11, 2022
@consideRatio consideRatio changed the title Fix compatibility with distributed >= 2022.5.1, traitlets >= 5.2.0, and click >= 8.1.3 and require it Fix compatibility with distributed >= 2022.5.1 and traitlets >= 5.2.0, and require versions that work Jun 11, 2022
@consideRatio
Copy link
Collaborator Author

@TomAugspurger I'd like to go for a release and get this resolved today if possible, is #573 (comment) reasonable to you?

Copy link
Member

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

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

Looks good thanks.

@consideRatio consideRatio changed the title Fix compatibility with distributed >= 2022.5.1 and traitlets >= 5.2.0, and require versions that work Fix compatibility with distributed >= 2022.5.1 and traitlets >= 5.2.0, and raise the lower bound of required versions Jun 13, 2022
@consideRatio consideRatio merged commit 2cb9d36 into dask:main Jun 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No changes in this repo, but our tests are now failing
2 participants