Don't validate http preloads locally #3768
Conversation
`validate_preload_argv` can't validate web address preloads for `click.Command` compliance, so it shouldn't try.
necaris
changed the title
There was a problem hiding this comment.
Thanks for the PR @necaris! Just for my own understanding, is the issue here that at the time the validation code is run, we haven't yet downloaded and parsed the remote module into a ModuleType object yet? Is this something that would be straightforward to test?
|
@jrbourbeau yes, that's basically the issue: because I presume that this could be tested by something along the lines of https://github.com/dask/distributed/blob/master/distributed/cli/tests/test_dask_scheduler.py#L269 -- does that seem like the right approach? |
Yes. We're using tornado's AsyncHTTPClient to download the module, and at the point where we validate we haven't yet spun up the event loop, and we're not in an asynchronous context. There are other inconsistencies with web-based preloads. In particular we don't import the module at setup time, but rather at start time. This is more-or-less necessary if we want to make it easy to test web-based preloads in an asynchronous context (which I think that we do). I think that testability is more important here than the feature mismatch. I'm inclined to wait for a user to arrive before we bend too much to make this fully consistent. |
Uses an arbitrary (hardcoded) local port and runs a separate Python process to serve that on localhost, then uses that address as the target of the `dask-scheduler` `--preload` option.
|
@jrbourbeau @mrocklin added 5b8c526 -- please let me know if that's a sensible testing approach. |
The scheduler_info.py module isn't around in the local directory It's only in memory. As a result we switch up the test a little Also we now use the tmpdir fixture rather than create tmp files with context managers
|
I've modified it a bit. I'm +1 on this and will plan to merge tomorrow if there are no objections. |
validate_preload_argvcan't validate web address preloads forclick.Commandcompliance, so it shouldn't try.