move backend append logic to the prepare_variable methods #1799
Conversation
| if LooseVersion(dask.__version__) > LooseVersion('0.8.1'): | ||
| da.store(self.sources, self.targets, lock=self.lock) | ||
| else: | ||
| da.store(self.sources, self.targets) |
There was a problem hiding this comment.
we now have a minimum dask version of 0.9 so this check is no longer needed.
| @@ -232,11 +227,8 @@ def set_variables(self, variables, check_encoding_set, | |||
| for vn, v in iteritems(variables): | |||
| name = _encode_variable_name(vn) | |||
| check = vn in check_encoding_set | |||
| if vn not in self.variables: | |||
There was a problem hiding this comment.
Can we deprecate the variables property as part of this change? That would help avoid this in the future. In general it's a bad idea for adapter layers (like our backend classes) to hold state.
There was a problem hiding this comment.
@shoyer - done. I actually removed the the attrs and dimensions properties too since they weren't really used. I left a deprecation error in there for now but can remove it assuming all the tests pass.
|
I'm glad to see we weren't using this properties very much internally :). Let's keep them as FutureWarning for now since there is a decent chance some third party backend subclass is using them, and there's no reason why we need to make things more difficult than necessary. |
|
@shoyer - this should be good to go following my last two commits. I'm not sure how to write a regression test for this but if you think its needed, I can try to whip something up. |
git diff upstream/master **/*py | flake8 --diff(remove if you did not edit any Python files)whats-new.rstfor all changes andapi.rstfor new API