Add type annotations to various functions within distributed.worker #5290
Conversation
|
Can one of the admins verify this patch? |
There was a problem hiding this comment.
Thanks for contributing @orf. cc @crusaderky in case you get a chance to look at this
| @@ -3259,8 +3267,7 @@ async def get_profile( | |||
| return prof | |||
|
|
|||
| async def get_profile_metadata(self, comm=None, start=0, stop=None): | |||
| if stop is None: | |||
| add_recent = True | |||
| add_recent = stop is None | |||
There was a problem hiding this comment.
Could you instead directly change to if stop is None: on line 3284?
There was a problem hiding this comment.
The line below this overwrites stop: stop = stop or now. I'm not clear on what these arguments do, but this would always make the condition true.
Co-authored-by: crusaderky <[email protected]>
Co-authored-by: crusaderky <[email protected]>
Co-authored-by: crusaderky <[email protected]>
There was a problem hiding this comment.
Thanks for the updates @orf! It looks like there is an ImportError in CI and the linting checks want to make code style updates.
For linting, you can see what checks are run here https://docs.dask.org/en/latest/develop.html#code-formatting. I'd recommend installing the optional pre-commit hooks to ensure these checks are always run locally prior to pushing up commits to GitHub.
|
Thanks for the suggestions! I've applied them. Sorry about the CI failures, I applied some suggestions directly from Github and I've only had time to do the other manual changes now. |
|
LGTM |
|
Is there anything I need to do to get this merged? It's unfortunate that it missed the 2021.09.0 release, as our users have been asking for this. The CI failure appears to be unrelated |
|
@jrbourbeau please merge |
There was a problem hiding this comment.
Thanks for the updates @orf and reviewing @crusaderky!
Also, @orf I noticed this is your first code contribution to this repository. Welcome!
|
Thank you! I'll go through and add some more annotations soon, there are some heavily used user-facing functions that could benefit a lot from it. I don't know the best place to ask this, but would you also accept contributions that added |
black distributed/flake8 distributed/isort distributedThis is my first contribution to this project, so I hope that it is OK! I've added type hints to
get_client(), the lack of which I found quite annoying when starting with Dask.I also added type hints to some other simple functions, and fixed a potential undefined local variable issue in
get_profile_metadata. I also removed some mutable default arguments, and added.venv/to the.gitignore.Please let me know if these are acceptable, or if you'd like any removed/changed!