-
Notifications
You must be signed in to change notification settings - Fork 12
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
Always write to tar file in serial #197
Always write to tar file in serial #197
Conversation
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.
I think we could revisit this problem if we have time in the future, but since it doesn't seem like there is a simple fail-proof way to enable writing to a tar file in parallel, it might be best just to avoid it.
I totally agree. Good thinking, and thanks for implementing it.
I gave a few minor comments, and can you add a what's new? In terms of tests, this seems like it would be difficult to test. I'm not convinced we need them here, but of course they'd be a nice addition if they are in fact easy enough.
aospy/automate.py
Outdated
@@ -295,19 +295,28 @@ def _exec_calcs(calcs, parallelize=False, client=None, **compute_kwargs): | |||
def func(calc): | |||
"""Wrap _compute_or_skip_on_error to require only the calc | |||
argument""" | |||
return _compute_or_skip_on_error(calc, compute_kwargs) | |||
return _compute_or_skip_on_error(calc, {'write_to_tar': False}) |
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.
This causes none of the other compute_kwargs to be passed. Maybe update the value instead?
compute_kwargs.update({'write_to_tar': False})
_compute_or_skip_on_error(calc, compute_kwargs)
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.
(I recognize that currently 'write_to_tar' is the only supported kwarg, so for now this is irrelevant. But this leaves the door open for other options in the future.)
aospy/automate.py
Outdated
else: | ||
return _submit_calcs_on_client(calcs, client, func) | ||
result = _submit_calcs_on_client(calcs, client, func) | ||
_serial_write_to_tar(calcs, **compute_kwargs) |
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.
Similar to above comment: replace **compute_kwargs
with write_to_tar=compute_kwargs['write_to_tar']
.
aospy/automate.py
Outdated
else: | ||
return [_compute_or_skip_on_error(calc, compute_kwargs) | ||
for calc in calcs] | ||
|
||
def _serial_write_to_tar(calcs, write_to_tar=True): | ||
if write_to_tar: |
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.
I think the logic is more intuitive if this if statement goes before the function call:
write_to_tar = compute_kwargs['write_to_tar']
if write_to_tar:
_serial_write_to_tar(calcs, write_to_tar=write_to_tar)
Or, if you want to keep the if statement where it is, make the function name '_maybe_serial_write_to_tar'
(which supersedes the above comment on L307)
Thanks @spencerahill, I agree with all your comments. I can't think of a great way to test this either (to some extent so long as our test suite doesn't produce empty header-related errors in the future, we should consider this PR to be successful). |
Thanks @spencerkclark ! Now just need that ongoing dask.distributed bug for 2.7 to be fixed upstream, and we'll finally be back to getting passing test suites :) |
While running the code for #228 I noticed a similar tar error:
It is possible that I am still seeing this message because I've been waiting for the custom reduction methods to be added before I upgrade. Does this mean that the data will occasionally fail to get written to tar, or can be it considered a false alarm? |
Yes, this seems like the same issue we resolved with this PR. Indeed, it means without this fix the data will occasionally fail to be written to the tar archive when calculations are submitted in parallel. Do you use the tar archive for anything? If you don't want to see these error messages, in lieu of updating to the master version of |
I keep the tar files as a backup, so it would be nice to have write_to_tar as True. If the custom reductions are going to be added in the near future, I'd wait for that. Otherwise, I agree that one of the options you suggested would work. |
On my end probably not until January at the earliest :( |
Unfortunately same here...I wouldn't count on those being implemented until 2018. |
@spencerahill this is a possible really basic workaround for #75. I don't think writing to a tar file is a particular bottleneck in our pipeline (all desired results are already computed and saved out to files at that point), so one solution is to always write to the tar file in serial (even if the computations themselves are done in parallel).
What are your thoughts on this? I think we could revisit this problem if we have time in the future, but since it doesn't seem like there is a simple fail-proof way to enable writing to a tar file in parallel, it might be best just to avoid it.