-
Notifications
You must be signed in to change notification settings - Fork 89
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
test-all #283
test-all #283
Conversation
@jcrist when running tests locally, I see notes from the go proxy like
Are those expected as part of the cluster being shut down? b9209a6 and fd8db22 are trying to clean up some strange behavior in I'm able to reproduce the other class of failures locally, but haven't been able to debug it yet
|
I think pytest-dev/pytest-asyncio#168 is the issue to track the Planning to merge when this passes. I'll open an issue to track unpinning pytest-asyncio. |
Some tests failing because of a change in distributed. I'm bisecting now, but have to step away for a while. |
Somehow dask/distributed#3928 broke |
Looking at the worker logs of the test
so it is a permissions issue with not being able to write to the directory where the |
@jcrist @TomAugspurger could this default empty string be the issue? Currently we're not able to launch gateway workers running distributed 2.20 (see linked issue)
|
Nice catch @scottyhq! Locally I have this.
I think the easiest short-term solution is for distributed to interpret all Falsey values like None, and look it up from the config / fall back to the cwd. I'll make a PR to distributed. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not the first time there have been issues with pytest-asyncio
but I'm surprised that it broke like this again so soon after a major-ish release.
I'll defer to @jcrist on this but this looks good -- working CI > broken CI
@@ -574,7 +574,7 @@ async def start_worker( | |||
nthreads=1, | |||
memory_limit="auto", | |||
scheduler_address=None, | |||
local_directory="", | |||
local_directory=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did distributed change how the local_directory
parameter is handled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this was dask/distributed#3441
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And for reference, dask/distributed#3964 is achieving the same goal (fall back to config / cwd) but by changing the implementation rather than the default. Dunno what the long term plan is.
LGTM, thanks @TomAugspurger! |
No description provided.