Deprecate mode argument to asdf.open

[drdavella]

As far as I can tell, this argument does not serve any useful purpose, but only allows for buggy behavior related to memory maps (see the discussion in #575 and #576).

For these reasons, I propose to remove it entirely. For the next few releases, using this argument will result in a warning. By 3.0, it should be removed entirely.

cc @vmarkovtsev

[vmarkovtsev]

+1 on this. Looks like it should always remain 'r' unless copy_arrays is False and the source is a file name or a "real" file descriptor.

[drdavella]

I'm not sure if copy_arrays should have any bearing on whether a file is writeable or not. There's no reason we can't update arrays and write them back out, even if they're not memory mapped.

Also I'm not sure why it shouldn't be possible to write back to an io.BytesIO. So maybe we should just open in rw by default unless the source is clearly an http address?

Maybe instead of deprecating the mode argument, we just need to enforce the following behavior: if mode='r', then copy_arrays=True. Otherwise we will raise an exception at open. And we can change the default to mode='rw' to be compatible with copy_arrays=False. We need to keep both arguments since the implication only goes in one direction: it's not the case that copy_arrays=True necessarily implies mode='r'.

[drdavella]

Better yet, I think the default should be mode=None and we should try to guess based on other input. If mode is explicitly provided by a user, we will perform some checks to ensure safe behavior.

[vmarkovtsev]

Makes much sense. The only situation when the code breaks is when the file is open readonly, copy_arrays is False and we try to change an array. We cannot ask users to not change arrays of course.

[drdavella]

Actually it might actually be better to force copy_arrays=True if mode='r', in order to avoid breaking current usage, but that's going to require some reworking of the internals.

This is just a turning into a giant pain, and I've dealt with this kind of thing in other libraries. This is the price we pay for having a generalized open function, though.

[vmarkovtsev]

We can always detect the mode of the file object and print a runtime warning where we set copy_arrays to True. I can work on that.

[drdavella]

You'll notice though that copy_arrays is used to create the block index before the other arguments to open are processed. I'm thinking of a bit more substantial overhaul where we make open a standalone factory method rather than a class method. In this case, use of AsdfFile.open would be deprecated in favor of asdf.open.

[drdavella]

Closing since we're no longer deprecating the mode, but instead we will add readonly protections when accessing memory-mapped arrays and the underlying file handle is readonly. See #579.