Skip to content
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

WIP enable user to specify .tar output or not and warn about incompatibilities w/ old tar version #160

Merged
merged 5 commits into from
Mar 28, 2017

Conversation

spencerahill
Copy link
Owner

@spencerahill spencerahill commented Mar 27, 2017

Closes #157

This first commit gets the functionality in place, namely that users can specify write_to_tar as a member of the newly created calc_exec_options dict that gets passed to submit_mult_calcs along with the other options that had been already being passed to submit_mult_calcs.

Tests and docs (including the warning about the old tar version on Macs) still needed, but @spencerkclark wanted to share this now in case you have any suggestions on the overall approach.

(Tied up for next couple hours but will return to this later today.)

@spencerkclark
Copy link
Collaborator

@spencerahill this looks very good. Thanks for adding docstrings to _exec_calcs and submit_mult_calcs. To try and help out, I went ahead added some tests. Hopefully I didn't step on any progress you already made!

@spencerahill
Copy link
Owner Author

@spencerkclark perfect, thanks! Will wrap up the rest of this by the end of the night.

@spencerahill
Copy link
Owner Author

spencerahill commented Mar 27, 2017

The engine='scipy' option to to_netcdf was added in c5864c2, and scipy was also added to the required dependencies and CI config files at some point. So all that's left is docs, tests, and what's new.

Also changed _compute_or_skip_on_error to skip on any exception,
rather than just RuntimeError.  I think this is better for the user,
and it makes testing it easier by making it easy to generate an
exception that this line will catch.

In order to test the logging, I am using the pytest-catchlog plugin,
which is one of pytest's official plugins.  I have added it to the CI
files and to setup.py; it's possible that further tinkering will be
required to get it to work with the CI.
@spencerkclark
Copy link
Collaborator

Also changed _compute_or_skip_on_error to skip on any exception,
rather than just RuntimeError. I think this is better for the user,
and it makes testing it easier by making it easy to generate an
exception that this line will catch.

Fully agree, thanks for making this change!

In order to test the logging, I am using the pytest-catchlog plugin,
which is one of pytest's official plugins. I have added it to the CI
files and to setup.py; it's possible that further tinkering will be
required to get it to work with the CI.

Really nice find on this. It's great to be able to have the logging tested too.

If you're satisfied I think this is ready for a what's new and then a merge.

@spencerahill
Copy link
Owner Author

@spencerkclark thanks! A little more in the way of docs + what's new, then will merge.

It's great to be able to have the logging tested too.

I agree, I have been wanting to figure this out for a while. Of course, getting it to work made me realize all the ways we could be doing logging better! Will try to gather my thoughts on that in an Issue sometime.

@spencerahill
Copy link
Owner Author

I am actually going to include the warning in the docs with the PR for #150...I have a local git stash where I had started in on that, and it will be easier to add this doc update along with that.

@spencerahill spencerahill merged commit f585b36 into develop Mar 28, 2017
@spencerahill spencerahill deleted the tar-output-bugfix branch March 28, 2017 03:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants