Skip to content

Centralize and type no_default#8171

Merged
crusaderky merged 1 commit into
dask:mainfrom
crusaderky:no_default
Sep 22, 2023
Merged

Centralize and type no_default#8171
crusaderky merged 1 commit into
dask:mainfrom
crusaderky:no_default

Conversation

@crusaderky

@crusaderky crusaderky commented Sep 7, 2023

Copy link
Copy Markdown
Collaborator

Comment thread distributed/client.py Outdated
FutureWarning,
)
n_workers = 0
elif not isinstance(n_workers, int) or n_workers < 1:

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

deprecated since Aug 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same here. These should be separate changes

Comment thread distributed/deploy/cluster.py Outdated
FutureWarning,
)
n_workers = 0
elif not isinstance(n_workers, int) or n_workers < 1:

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Deprecated since Aug 2022

Comment thread distributed/tests/test_utils.py Outdated
await asyncio.gather(set_var("foo"), set_var("bar"))


def test_serialize_for_cli_deprecated():

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

All these were deprecated since 2021

@github-actions

github-actions Bot commented Sep 7, 2023

Copy link
Copy Markdown
Contributor

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

       21 files  ±  0         21 suites  ±0   11h 13m 7s ⏱️ + 40m 10s
  3 814 tests +  1    3 704 ✔️ +  1     107 💤 ±0  3 ±0 
36 865 runs  +10  35 062 ✔️ +11  1 800 💤 ±0  3  - 1 

For more details on these failures, see this check.

Results for commit b972e31. ± Comparison against base commit 1650ceb.

♻️ This comment has been updated with latest results.

Comment thread distributed/utils.py Outdated
"parse_bytes": "dask.utils.parse_bytes",
"parse_timedelta": "dask.utils.parse_timedelta",
"typename": "dask.utils.typename",
"tmpfile": "dask.utils.tmpfile",

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

All these were deprecated since 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In the interest of a meaningful changelog, I'm inclined to split off these changes. The title of this PR will end up in the changelog and nothing in there says anything about removing deprecated functionality or configuration

@crusaderky crusaderky force-pushed the no_default branch 2 times, most recently from b9fa07d to 683837e Compare September 14, 2023 17:22
@crusaderky crusaderky mentioned this pull request Sep 15, 2023
4 tasks
@crusaderky crusaderky force-pushed the no_default branch 3 times, most recently from c1f4e15 to 69ed182 Compare September 18, 2023 10:36

@fjetter fjetter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't mind the changes themselves but I would like us to adopt a better habit of communicating these changes properly. The mechanism right now is to put PR titles into the changelog.

Comment thread distributed/utils.py Outdated
"parse_bytes": "dask.utils.parse_bytes",
"parse_timedelta": "dask.utils.parse_timedelta",
"typename": "dask.utils.typename",
"tmpfile": "dask.utils.tmpfile",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In the interest of a meaningful changelog, I'm inclined to split off these changes. The title of this PR will end up in the changelog and nothing in there says anything about removing deprecated functionality or configuration

Comment thread distributed/client.py Outdated
FutureWarning,
)
n_workers = 0
elif not isinstance(n_workers, int) or n_workers < 1:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same here. These should be separate changes

@crusaderky

Copy link
Copy Markdown
Collaborator Author

@fjetter opened #8192 and #8193

@crusaderky crusaderky force-pushed the no_default branch 2 times, most recently from 6400e84 to 43cbe2f Compare September 18, 2023 14:16
@crusaderky

Copy link
Copy Markdown
Collaborator Author

@fjetter ready for review again

@phofl phofl left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

good to merge

@crusaderky crusaderky merged commit b6333df into dask:main Sep 22, 2023
@crusaderky crusaderky deleted the no_default branch September 22, 2023 14:26
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.

3 participants