Fix the incorrect usage of repository interface of aiida-core

[sphuber]

Fixes #548

The following illegal uses have been fixed:

• Node.open has to be use in a context manager
• Node._get_base_folder will soon be removed and cannot be used.

[sphuber]

I am putting this on hold. This actually serves as an example for other plugin developers for fixes they most likely have to apply to their plugins for them to be compatible with the upcoming new version of aiida-core that will have the new repository implementation. Since the behavior I am correcting here will break with the new version (and we cannot provide a backwards compatible transition period) we are going to already alert them and suggest they update the plugins ahead of time.

The current plan we are discussing is to add deprecation warnings in aiida-core==1.4 and then add the new repository implementation in aiida-core==1.5. If all plugins can update between v1.4 and v1.5, there should be no problem.

[greschd]

Is it just the _repository._get_base_folder().abspath construction that would be deprecated, or also using Node.open() without a context manager?

Of course a bare Node.open is not nice because it leaves the file open (well, until the garbage collector gets around to it), but since you can manually .close() it I'd probably be in favor of still allowing it (similar to the regular Python open).

As for _repository._get_base_folder()... I mean, with a double protected access you're kinda asking for that to be broken 😄

[sphuber]

Of course a bare Node.open is not nice because it leaves the file open (well, until the garbage collector gets around to it), but since you can manually .close() it I'd probably be in favor of still allowing it (similar to the regular Python open).

Unfortunately, the way the new repository is currently implemented, this will be difficult. The reason is that the disk-object-store library, implemented by Giovanni, which provides our efficient long term storage, requires use of context manager, because it allows it to make the implementation more efficient. We would have to ask @giovannipizzi if it is possible and worth reverting this requirement and allowing also non-contextual use of open.

As for _repository._get_base_folder()... I mean, with a double protected access you're kinda asking for that to be broken

Sure, except that aiida-quantumespresso was also using it, which many plugin developers have used as the example over the years 😅
I guess we should have clearly stated "Do as I say, not as I do" ;)

[greschd]

Unfortunately, the way the new repository is currently implemented, this will be difficult. The reason is that the disk-object-store library, implemented by Giovanni, which provides our efficient long term storage, requires use of context manager, because it allows it to make the implementation more efficient. We would have to ask @giovannipizzi if it is possible and worth reverting this requirement and allowing also non-contextual use of open.

Ah, I see. Well, I don't really see a use case where it would be necessary to manually open / close instead of using the context manager, so I guess it's fine to deprecate that.
We should probably make sure that manual open / close gives a reasonable traceback, though: Since the new implementation (I presume) will still have a .open() method that just returns a context manager, it might result in a confusing error when e.g. trying to read or iterate over that object without __enter__ing it first.

Sure, except that aiida-quantumespresso was also using it, which many plugin developers have used as the example over the years 😅
I guess we should have clearly stated "Do as I say, not as I do" ;)

Yeah, true.. definitely best to proactively check who is using it and warn them.

and we cannot provide a backwards compatible transition period

That just means the old interface won't work with the new repository, right? As in, the updated code will still be compatible with older (1.0+) AiiDA versions?
I had somehow interpreted this as "there is no interface that works with new (>=1.5) and old (<=1.4) AiiDA versions simultaneously" before, which would be more serious.

[sphuber]

We should probably make sure that manual open / close gives a reasonable traceback, though: Since the new implementation (I presume) will still have a .open() method that just returns a context manager, it might result in a confusing error when e.g. trying to read or iterate over that object without __enter__ing it first.

Yeah, currently you can get something like:

 AttributeError: '_GeneratorContextManager' object has no attribute 'read'

That just means the old interface won't work with the new repository, right? As in, the updated code will still be compatible with older (1.0+) AiiDA versions?
I had somehow interpreted this as "there is no interface that works with new (>=1.5) and old (<=1.4) AiiDA versions simultaneously" before, which would be more serious.

If with "updated code" you mean the updated aiida-quantumespresso after this PR, then yes. The changes here will make the code compatible with current aiida-core as well as aiida-core~=1.5 once we merge the new interface. There are some other minor changes, such as moving some imports, changing some methods argument names and changing type from enum to class (I know, we did this before thinking it was easy ;) ). I will make a PR soon that already adds these changes with just the deprecation warnings such that it can be merged in v1.4.

Unfortunately, due to the code fixed in this PR that many plugins will have, it will be impossible to have older plugin versions backward compatible with the new repository interface. The use of _get_base_folder to get the abspath will be impossible to provide with the new repository.

Would you be interested in giving a look at my repo branch to see if you can spot potential backwards incompatibilities that I introduced that I forgot about? In any case, I think once I finalized it, I will send a mail to plugin developers notifying them about the plans, this PR and the changes they should prepare themselves and then have them test their plugin against my branch.

All input and comments to the plan are welcome to try and make this as smooth as possible.

[greschd]

If with "updated code" you mean the updated aiida-quantumespresso after this PR, then yes. The changes here will make the code compatible with current aiida-core as well as aiida-core~=1.5 once we merge the new interface. There are some other minor changes, such as moving some imports, changing some methods argument names and changing type from enum to class (I know, we did this before thinking it was easy ;) ). I will make a PR soon that already adds these changes with just the deprecation warnings such that it can be merged in v1.4.

Unfortunately, due to the code fixed in this PR that many plugins will have, it will be impossible to have older plugin versions backward compatible with the new repository interface. The use of _get_base_folder to get the abspath will be impossible to provide with the new repository.

Yeah, I mean at least there's a happy updating path, and with a deprecation period.. not ideal, but I'd say good enough :) At least it doesn't force users to upgrade aiida-core if they want newer plugins (unless the plugin itself decides so).

Would you be interested in giving a look at my repo branch to see if you can spot potential backwards incompatibilities that I introduced that I forgot about? In any case, I think once I finalized it, I will send a mail to plugin developers notifying them about the plans, this PR and the changes they should prepare themselves and then have them test their plugin against my branch.

Sure, I can give it a glance. Which branch is that?

[sphuber]

Sure, I can give it a glance. Which branch is that?

https://github.com/sphuber/aiida_core/tree/fix/3445/disk-object-store-repository

Note that I wrote this without trying to be fully backwards compatible. Since we now decided that we still want to try, in the latest commit I started "adding" backward compatibility stuff. But what I will have to do soon, is take those affected resources and already put those deprecations on develop in a separate PR. The list probably is not yet complete, would be good if you find some more.

[greschd]

I've added a few comments here: greschd/aiida_core#5 (review)

By no means a guarantee that I found all backwards-incompatible bits.

[greschd]

I think it would be ok to merge this now - it should be compatible with 1.3+ and (as far as we know now) 2.0, right?

If we want to point other plugins here as an example, there's no harm in it being merged already.

[sphuber]

Yeah this could be merged. I was just keeping it open as an example, but we could just as easy refer to it when it is closed. Will update it

[greschd]

Good to go for me, just one minor comment.