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

Drop comparison of versions against all clients #6861

Merged

Conversation

hendrikmakait
Copy link
Member

@hendrikmakait hendrikmakait commented Aug 9, 2022

Closes #6536

New logged warning (in test_version_warning_in_cluster):

2022-08-10 15:42:31,352 - distributed.worker - WARNING - Mismatched versions found

+---------+---------------------------------------------+----------------------+-----------------------------------+
| Package | Worker-5a4690f1-71ca-4c19-9b3b-22ac5c13674a | Scheduler            | Workers                           |
+---------+---------------------------------------------+----------------------+-----------------------------------+
| dask    | 2022.8.0+5.gf7fd6c48                        | 2022.8.0+5.gf7fd6c48 | {'2022.8.0+5.gf7fd6c48', '0.0.0'} |
+---------+---------------------------------------------+----------------------+-----------------------------------+

Out of scope:

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

@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2022

Unit Test Results

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

       15 files  +       15         15 suites  +15   6h 19m 26s ⏱️ + 6h 19m 26s
  2 992 tests +  2 992    2 901 ✔️ +  2 901       88 💤 +     88  3 +3 
22 189 runs  +22 189  21 137 ✔️ +21 137  1 049 💤 +1 049  3 +3 

For more details on these failures, see this check.

Results for commit 7a50df1. ± Comparison against base commit 2e928ec.

♻️ This comment has been updated with latest results.

@hendrikmakait
Copy link
Member Author

CI flake: #6506

@hendrikmakait hendrikmakait marked this pull request as ready for review August 10, 2022 11:06
versions,
client_name="This Worker",
source_name=f"Current worker: {ws.server_id}",
Copy link
Member

Choose a reason for hiding this comment

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

Does asciitable render a newline properly? Current worker:\n{ws.server_id}
This way we'd reduce horizontal space of the warning by 16 chars

Copy link
Member Author

@hendrikmakait hendrikmakait Aug 10, 2022

Choose a reason for hiding this comment

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

Let me take a look. IMO, we could also just skip Current worker: entirely. The source should be sufficiently identified by its ID. My guess would be that the formatting wreaks havoc on the formatting since it should break in the middle of the table header.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, that just messes things up. I'll remove Current worker: so that the message will look like this:

2022-08-10 15:42:31,352 - distributed.worker - WARNING - Mismatched versions found

+---------+---------------------------------------------+----------------------+-----------------------------------+
| Package | Worker-5a4690f1-71ca-4c19-9b3b-22ac5c13674a | Scheduler            | Workers                           |
+---------+---------------------------------------------+----------------------+-----------------------------------+
| dask    | 2022.8.0+5.gf7fd6c48                        | 2022.8.0+5.gf7fd6c48 | {'2022.8.0+5.gf7fd6c48', '0.0.0'} |
+---------+---------------------------------------------+----------------------+-----------------------------------+

@@ -105,15 +105,15 @@ def get_package_info(
return pversions


def error_message(scheduler, workers, client, client_name="client"):
Copy link
Member Author

Choose a reason for hiding this comment

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

Driveby: Renamed client to source since the source we want to compare against might be a worker or a client.

Copy link
Member

@fjetter fjetter Aug 10, 2022

Choose a reason for hiding this comment

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

That's fine, I don't think anybody is using this outside of this repo

versions,
client_name="This Worker",
source_name=f"Current worker: {ws.server_id}",
Copy link
Member Author

@hendrikmakait hendrikmakait Aug 10, 2022

Choose a reason for hiding this comment

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

Let me take a look. IMO, we could also just skip Current worker: entirely. The source should be sufficiently identified by its ID. My guess would be that the formatting wreaks havoc on the formatting since it should break in the middle of the table header.

@hendrikmakait hendrikmakait requested a review from fjetter August 10, 2022 15:26
@fjetter fjetter merged commit 99a2db1 into dask:main Aug 11, 2022
gjoseph92 pushed a commit to gjoseph92/distributed that referenced this pull request Oct 31, 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.

Version warning is blending client/worker versions
2 participants