get_versions without importing packages
#5724
Conversation
|
Do we want to use the backport? https://github.com/python/importlib_metadata |
pkg_resources.get_distribution("numpy").version |
|
The Python docs seem to suggest
The pkg_resources docs say the same:
I guess instead of requiring the backport, I could use |
xfail'd until dask#5724
|
The number of flaky-seeming test failures here is concerning me. Could it be that not importing pandas on the workers is allowing them to start faster, and making race conditions even more likely to happen? Or something like that? |
|
I don't get it - I thought that in order to not import pandas we need both this and #5695? |
|
Looking at the runtimes of the heavily flaky test suites, I'm seeing 1h+ runtime instead of the usual 30min. Which is something I've seen in the past on main. So I suspect it may be github's fault. |
Totally correct—just forgot, sorry. |
|
Merged CI test of #5695 + #5724 running on crusaderky#2 and things don't look very good so far. I can see much more flakiness than main and much slower end-to-end test runtime. |
|
CI is back on track; I've rerun all tests. Failed tests (with number of failures): |
|
@gjoseph92 please merge from main |
|
please
|
|
Latest run vs main: https://github.com/crusaderky/distributed/actions/runs/1875755877 Failures: |
Co-authored-by: crusaderky <[email protected]>
|
@crusaderky comments addressed |
Co-authored-by: Thomas Grainger <[email protected]>
Co-authored-by: Thomas Grainger <[email protected]>
Co-authored-by: Thomas Grainger <[email protected]>
Co-authored-by: Thomas Grainger <[email protected]>
|
So it turns out main importlib |
|
I've handled merge conflicts and pushed to your branch. I hope that you don't mind @gjoseph92 |
This is still happening. I propose that instead maybe this change adds a non-trivial and somewhat stochastic cost to startup time, which has genuinely increased the duration of the test suite.
Yeah, I like this way of thinking. Rather than try to hide the problem, let's expose it more clearly. However, looking through the test failures that I'm seeing here (just anecdotally skimming) I'm seeing that half of them are things that I often see in practice, and half of them are, I think, unrelated. At first I was very excited about this PR because it shined a bright light on all of the bad tests. However now I'm concerned that it might not be doing a good job of prioritizing them for us. I think that I still value just going through existing PRs and picking off failing tests from them and seeing if we can solve them. That's less clear signal, but it's also less biased signal. |
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 14 files - 1 14 suites - 1 15h 6m 29s ⏱️ + 8h 59m 24s For more details on these failures and errors, see this check. Results for commit c72f970. ± Comparison against base commit 7b24c94. |
|
This is still very red after merging from main. |
5 participants
This makes the version-compatibility check no longer actually import the packages being checked.
It also removes support for custom version-getting functions, and only uses
importlib.metadata.version(EDIT: orpkg_resourceson 3.7) to look up the version number registered for a package.I assumed the purpose of custom version-getting functions was to handle discrepancies in how modules expose their version as an attribute, like how most use
.__version__, but tornado uses.version, msgpack uses.versionbut it's a tuple of ints instead of a string, etc. That becomes irrelevant withimportlib.metadata.version, which will always give you the version number of any reasonably-formed package as a string.You could use this
(modname, version_getter_func)form as an argument toClient.get_versions, so not supporting the tuple form in theory is a breaking change to a public API. However, that form isn't documented, so I don't know if that counts.If we want to maintain support for those custom functions, I will, it just makes the code messier.
pre-commit run --all-files