Skip to content

Conversation

@crusaderky
Copy link
Collaborator

@crusaderky crusaderky commented Jun 6, 2022

@crusaderky crusaderky changed the title Don't catch ValueError in Worker._transition Don't invoke log_event from state machine Jun 6, 2022
@crusaderky crusaderky linked an issue Jun 6, 2022 that may be closed by this pull request
@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2022

Unit Test Results

       15 files  ±0         15 suites  ±0   6h 33m 47s ⏱️ + 2m 53s
  2 851 tests  - 1    2 768 ✔️  -   3    81 💤 ±0  2 +2 
21 121 runs   - 8  20 173 ✔️  - 12  946 💤 +2  2 +2 

For more details on these failures, see this check.

Results for commit ef12618. ± Comparison against base commit bd98e66.

♻️ This comment has been updated with latest results.


del s.events["invalid-worker-transition"] # for test cleanup


Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Duplicates test in test_utils_test.py

@crusaderky
Copy link
Collaborator Author

Ready for final review and merge

@fjetter fjetter merged commit bde90af into dask:main Jun 7, 2022
@crusaderky crusaderky deleted the merge_recs_error branch June 7, 2022 14:55
Comment on lines +6230 to +6231
if isinstance(msg, dict):
msg["worker"] = worker
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious, why this change?

For context, this tripped me up when doing some Coiled work. I'm not against it, I'm just curious about the motivation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Previously, the same info was manually added to most worker-side calls to log-event. Adding it when it lands looks cleaner and less redundant.

Copy link
Member

Choose a reason for hiding this comment

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

Users can use this system too though. We're adding this to their messages.

Copy link
Member

Choose a reason for hiding this comment

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

Also, when do you sleep? I'm about to finish up my work day in California. If you're working past when I do then that really puts me to shame :)

Copy link
Member

Choose a reason for hiding this comment

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

(travelling away from home in Texas this week)

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.

Yank state machine out of Worker class

3 participants