aiidateam / aiida-core

The official repository for the AiiDA code
https://aiida-core.readthedocs.io
Other
435 stars 188 forks source link

Add `Parser.retrieved_temporary_folder` to easily access it, like `retrieved` #3502

Open sphuber opened 5 years ago

sphuber commented 5 years ago

Currently, if defined, it is passed in the keyword arguments of the Parser.parse call and the user has to look for it there. Maybe making it accessible through a property is more convenient. Question would be if that should return a simple absolute filepath (like currently in the keyword arguments) or maybe make it a FolderData instance to give it the same interface as self.retrieved.

espenfl commented 5 years ago

I support this. Would be nice to use it precisely as with self.retrieved as it would be a drop in replacement.

One could even imagine that everything was dropped in the same folder (both the permanent and temporary content). We have access to the lists that contain the definitions etc. in case one need to check (but the plugin devs should already have full control over this).

sphuber commented 5 years ago

One could even imagine that everything was dropped in the same folder (both the permanent and temporary content).

This won't be possible, because self.retrieved points to the FolderData output node retrieved, the content of which is permanently stored. The whole point of retrieve_temporary_folder was to have a way to retrieve files that are not stored permanently.

espenfl commented 5 years ago

What I meant was not the same FolderData but the same self.retrieved property (whatever that is, now it is a link to FolderData, but does not have to be). But let us go with two separate access points to keep it clear.

espenfl commented 3 years ago

@sphuber @chrisjsewell What is the status of this. Is this something we can consider for the v2 release? Mentioning @zhubonan as well.

sphuber commented 3 years ago

I think we can definitely provide it through a property on the parser. However, what to do with the interface is not absolutely clear. I know I suggested to wrap it in a FolderData instance, but that may give the wrong idea as in it will be storable whereas the whole point of the temporary folder is that it shouldn't be stored. When we address this, whatever solution we come up with, would be worthwhile to also consider #4783

zhubonan commented 3 years ago

but that may give the wrong idea as in it will be storable whereas the whole point of the temporary folder is that it shouldn't be stored.

Might be a silly idea - would it make sense to have something like TransientFolderData (sub-class of FolderData) which throws error when store is called? I am not quite familiar with the new object store - does it commit the data to the database before the node is stored? We should avoid having the data written to the actual repository and later deleted... Maybe such TransientFolderData can be linked to a temporary repository?

chrisjsewell commented 3 years ago

Yeh no, I think let's just keep it simple and return it as a pathlib.Path object, because that is what it is, just a path, i.e. having no relationship to the aiida database or repository etc

espenfl commented 3 years ago

Yeh no, I think let's just keep it simple and return it as a pathlib.Path object, because that is what it is, just a path, i.e. having no relationship to the aiida database or repository etc

What would be nice is to move the boilerplate we have to have on the plugin side to core if we use both retrieved and retrieved_temporary_folder. We then have to do e.g. list_objects() and something like os.listdir() etc. Being able to issue e.g. open on some returned names on the temporary would make it easier for the plugin side and possibly give you more flexibility to change this in core. But I fully support keeping it simple. Do you know if the other plugins utilize this?

chrisjsewell commented 3 years ago

Well basically these days, everything that acts like a path in Python should try to implement the pathlib.PurePath interface, and this goes the same for FolderData. Somewhat related, I have already played around with making the repository interface PurePath compliant: https://github.com/aiidateam/aiida-core/pull/4938