[WIP] Download preload urls in the Preload constructor #5194
Conversation
Turn _download_module into a sync function Sort imports
|
/cc @mrocklin finally some progress. It only failed in |
|
Let's try adding a longer timeout? |
|
Nope. Still a problem. |
|
hhhmmm 😞 that's the only env where these tests are failing. Unfortunately I don't have much macos experience to know what's going on here. I'll keep looking later today. |
|
We can also add a version dependent skip. This is probably past the point
of diminishing returns.
…On Tue, Aug 10, 2021 at 9:05 AM Marcos Moyano ***@***.***> wrote:
hhhmmm 😞 that's the only env where these tests are failing.
Unfortunately I don't have much macos experience to know what's going on
here. I'll keep looking later today.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5194 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACKZTAH36XURP7TOIJ5YTTT4EW2ZANCNFSM5B3UN77A>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
|
That seemed to have done it. The failing tests don't look related to my changes. |
| @@ -119,13 +119,14 @@ def _import_module(name, file_dir=None) -> ModuleType: | |||
| return module | |||
|
|
|||
|
|
|||
| async def _download_module(url: str) -> ModuleType: | |||
| def _download_module(url: str) -> ModuleType: | |||
There was a problem hiding this comment.
Apologies for being late to the party. Could you elaborate a bit more on why this change was needed? My guess is there was a use case where we wanted the preload to be downloaded before starting up a worker or nanny. But I personally would find a little more explanation for this change useful
There was a problem hiding this comment.
Preloads have actions that happen on ...
- Import
- Setup
- Teardown
Except if the preload is a web preload, in which case the import happens at start. It's an odd exception that this PR removes.
For the practical use case this has to do with workers asking another service what the scheduler address is when they start up.
There was a problem hiding this comment.
Thanks Matt for the quick reply. I definitely need to do a better job adding context to my PRs
There was a problem hiding this comment.
No worries @marcosmoyano! Thanks for the additional details @mrocklin
|
This PR implicitly adds xref this CI build over in |
it was just for convenience. I'm sorry I thought urllib3 was already a dependency. I'll create a quick PR using |
|
That would be great, thank you @marcosmoyano! |
We accidentally introduced urllib3 as a dependency in #5194. This PR fixes that mistake.
black distributed/flake8 distributed/isort distributedFollow up from: #5185
Go back to processes and waiting a bit until the process and the webserver are up.
I'll leave #5185 open for a bit and try to figure out what's the difference between the environments.