Skip to content

Add dummy factory methods for ExecuteSuccessEvent and ExecuteFailureEvent#6687

Merged
crusaderky merged 5 commits intodask:mainfrom
hendrikmakait:dummy-methods
Jul 7, 2022
Merged

Add dummy factory methods for ExecuteSuccessEvent and ExecuteFailureEvent#6687
crusaderky merged 5 commits intodask:mainfrom
hendrikmakait:dummy-methods

Conversation

@hendrikmakait
Copy link
Member

@hendrikmakait hendrikmakait commented Jul 7, 2022

Addresses #6676 (comment)

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

@github-actions
Copy link
Contributor

github-actions bot commented Jul 7, 2022

Unit Test Results

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

       15 files  ±  0         15 suites  ±0   6h 13m 51s ⏱️ - 16m 31s
  2 920 tests +  2    2 839 ✔️ +  2    79 💤  - 1  2 +1 
21 619 runs  +13  20 685 ✔️ +20  932 💤  - 8  2 +1 

For more details on these failures, see this check.

Results for commit 6a15062. ± Comparison against base commit f7f6501.

else:
TaskStateState = str

Type = type
Copy link
Collaborator

Choose a reason for hiding this comment

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

What mypy madness makes this necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

mypy complains about type being used as a type for type and pyupgrade force-corrects from typing import Type.

Copy link
Member Author

Choose a reason for hiding this comment

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

Specifically, I think mypy is confused by type also being an instance variable on the dataclass.

Copy link
Collaborator

Choose a reason for hiding this comment

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

but how comes it worked before?!?

Copy link
Member Author

Choose a reason for hiding this comment

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

mypy only got confused in the scope of dummy, where it suddenly had 3 options for type:

  • instance variable
  • type class
  • function parameter

start: float = 0.0,
stop: float = 1.0,
nbytes: int = 8,
type_: Type | None = None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
type_: Type | None = None,
type: Type | None = None,

I think ideally, dummy would have the same signature as the non-dummy __init__.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, this was a remnant from mypy complaining about type being used as a type for type. I double-checked and switching the variable name is not a problem.

@crusaderky
Copy link
Collaborator

I removed the option to pass fields like exception, start, stop, etc. as they're all inconsequential to the worker state

@crusaderky crusaderky merged commit 533f082 into dask:main Jul 7, 2022
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