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

lock kwarg needs a deprecation cycle? #5073

Closed
fmaussion opened this issue Mar 24, 2021 · 6 comments · Fixed by #5256
Closed

lock kwarg needs a deprecation cycle? #5073

fmaussion opened this issue Mar 24, 2021 · 6 comments · Fixed by #5256

Comments

@fmaussion
Copy link
Member

Salem's tests on master fail because I use the lock kwarg to open_dataset, which seems to have disappeared in the backend refactoring.

Should the new open_dataset simply ignore lock, and raise a FutureWarning when used?

@max-sixty
Copy link
Collaborator

Yes that sounds right, thanks for spotting @fmaussion . (I am hazy on the differences but I think it's a DeprecationWarning since we're removing it rather than changing its behavior)

@fmaussion
Copy link
Member Author

I think it's a DeprecationWarning since we're removing it rather than changing its behavior

Ha! I'm no expert either. I learned one thing recently though: DeprecationWarnings are silenced by python per default when not raised directly from __main__ and are therefore invisible for many users.

https://stackoverflow.com/questions/66590950/deprecationwarning-is-not-raised-on-import

https://stackoverflow.com/questions/55377831/difference-between-deprecationwarning-pendingdeprecationwarning-and-futurewarni

@keewis
Copy link
Collaborator

keewis commented Mar 25, 2021

as far as I understand the docs of DeprecationWarning and FutureWarning, the main difference is the intended target audience in a traditional library vs. application setting (where libraries are always used to write applications): PendingDeprecationWarning and DeprecationWarning are used to signal deprecations in the library, while FutureWarning are intended for users of the application. Interestingly, PEP565 mentions that FutureWarning can also be used for changed semantics of valid code.

If we follow that, xarray should not issue FutureWarning (other than to report changing semantics) because it is a library and both interactive and scripted use would be similar to writing a application.

@dcherian
Copy link
Contributor

We should fix this before the next release.

@alexamici
Copy link
Collaborator

Sorry for being so late to the party! I thought @aurghs did respond to this issue and forgot about it.

What has happened is not that lock (and group for the matter) disappeared altogether, but it has been moved to be a backend specific keyword argument. As far as I can see most internal backends still support it as before: cfgrib, netcdf4, pseudonetcdf, pynio and scipy.

The main difference is that lock was silently ignored by backend that don't support it like zarr and pydap.

The solution is to in fact only advertise the deprecation for those backends in my opinion as the functionality is legit for most of the backends.

Sorry again for seeing this so late.

@TomNicholas
Copy link
Member

No worries @alexamici ! I've merged your PR and it automatically closed this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants