CalcJob: add the option to stash files after job completion

[sphuber]

Fixes #2150

Tests and documentation still to be added once API is agreed upon.

[codecov]

Codecov Report

Merging #4424 (f05e9ea) into develop (241f251) will decrease coverage by 0.08%.
The diff coverage is 56.48%.

@@             Coverage Diff             @@
##           develop    #4424      +/-   ##
===========================================
- Coverage    79.57%   79.50%   -0.07%     
===========================================
  Files          515      519       +4     
  Lines        36941    37076     +135     
===========================================
+ Hits         29392    29473      +81     
- Misses        7549     7603      +54     

Flag	Coverage Δ
django	74.24% <56.48%> (-0.05%)	⬇️
sqlalchemy	73.15% <56.48%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida/engine/daemon/execmanager.py	62.79% <3.58%> (-6.96%)	⬇️
aiida/orm/nodes/data/remote/base.py	65.96% <14.29%> (ø)
aiida/engine/processes/calcjobs/tasks.py	65.43% <24.49%> (-5.95%)	⬇️
aiida/engine/processes/calcjobs/calcjob.py	86.43% <89.29%> (+0.95%)	⬆️
aiida/orm/nodes/data/remote/stash/folder.py	96.43% <96.43%> (ø)
aiida/common/datastructures.py	96.88% <100.00%> (+0.33%)	⬆️
aiida/orm/nodes/data/__init__.py	100.00% <100.00%> (ø)
aiida/orm/nodes/data/remote/__init__.py	100.00% <100.00%> (ø)
aiida/orm/nodes/data/remote/stash/__init__.py	100.00% <100.00%> (ø)
aiida/orm/nodes/data/remote/stash/base.py	100.00% <100.00%> (ø)
... and 6 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 241f251...f05e9ea. Read the comment docs.

[giovannipizzi]

Some suggestion for changes (I didn't do a complete review)

[ramirezfranciscof]

I think I am a bit wary of introducing more explicit file handling inside of the engine. I rememeber that not so far ago we were discussing some limitations that were the result of the engine copying files directly instead of using the methods of RemoteData. Was this for extending the copying of files from one remote machine to another? I don't remember now; I do remember the underlying issue was that these methods would open and close transports in an inefficient way and that it might be convenient to have methods that USE already open transports.

Perhaps if we were to deal with that now (since we are anyways modifying the RemoteData class) we could minimize the logic contained inside of the engine, who would just orchestrate the methods of the target remote node. Moreover, this would also prevent having the stash and unstash logic separated (so both would live inside the respective remote class instead of being one in the engine and the other inside the transfer calcjob).

So instead of having a RemoteFolderData where we run jobs and RemoteStashData where we stash them, we would have a RemoteFolderData to run jobs but which could also be used as a class for the stashing (if you just copy), or you could also stash in a RemoteZipData, or a RemoteScriptData (which uses a bashscript for its methods), etc.

Does this idea make any sense?

[sphuber]

I think I am a bit wary of introducing more explicit file handling inside of the engine. I rememeber that not so far ago we were discussing some limitations that were the result of the engine copying files directly instead of using the methods of RemoteData. Was this for extending the copying of files from one remote machine to another? I don't remember now; I do remember the underlying issue was that these methods would open and close transports in an inefficient way and that it might be convenient to have methods that USE already open transports.

Yes, the problem with the current implementation of RemoteData is that it will open a new connection itself. If this code is then directly used in processes, if they are run in large quantities, you can easily open too many connections and get banned. You could change the implementation to receive an open connection that it then uses, but then you are simply moving the problem. The code that calls the RemoteData is then responsible for opening a connection. If this is not done in a clever way, you end up with the same problem. The advantage of having the engine do these things, is that it has all the necessary infrastructure such as the transport queue.

Note that this is not the only complexity that the engine does well that would be difficult to reproduce by custom code. What happens if your call over transport fails, because the connection is interrupted. The engine has again complex functionality in the EBM to automatically retry and pause if the error persists. Recreating this behavior in client scripts is not trivial. I don't think that you can get away from the fact that if you want to perform tasks over transport, you will have to write asynchronous code that for most users will not be trivial. It is for that reason that I don't think we can make the RemoteData have a simple interface to do operations on. At least not when used in automated processes. For manual use in let's say a shell or command line, that would be different. But you wouldn't be able to just use it in your work chains or calculation jobs. I am not sure if all this is clear.

So instead of having a RemoteFolderData where we run jobs and RemoteStashData where we stash them, we would have a RemoteFolderData to run jobs but which could also be used as a class for the stashing (if you just copy), or you could also stash in a RemoteZipData, or a RemoteScriptData (which uses a bashscript for its methods), etc.

I think this refers more to issue #4626 does it? As I responded there to one of Leo's comments, I don't think the class should be determined by what type of stashing was used. It should rather distinguish it from a RemoteFolderData in the sense that that one can be readily accessed, whereas the RemoteStashData might not be, because it may be archived, written to tape, etc.

Does this idea make any sense?

That depends. If I understand your concern correctly (correct me if I am wrong) you do not like the concept that the interaction with this remote data has to be done indirectly through the engine. Instead, you would prefer to see this functionality directly accessible and usable through the various data classes. I hope I have explained clearly why I think that this approach won't work for cases where these actions have to be done in an automated way, that is to say in workchains. Of course for the single manual operation it might work, but I doubt that this is the majority of the use cases.

[giovannipizzi]

To comment on the discussion above: I think this might be right moment to change the interface of the RemoteData concerning auto-opening transports.
I would add a parameter transport to all concerned functions, e.g. is_empty, getfile, listdir, listdir_withattributes, _clean.
For backward compatibility, we make it optional for now - if not specified, we open a transport but also raise a deprecation warning. In 2.0, we make the parameter compulsory.

We instead add a get_transport method to do it (I would make it public, I think it's convenient for the users - this is the reason we were doing it "implicitly").

If possible (but I'm not sure it's easy) we should also check that the transport is "correct", i.e. to the correct machine (but I'm not sure it's easy).

Reason: while I agree that this only shifts the problem, it shifts it in the right direction: now, a conscious user cannot avoid to open a lot of connections. After this change, you can at least do something like:

• get a list of remote data, group by computer
• open the transport once
• call all methods, for all remote data on the same computer, within the same connection

This will open up the possibility in the future to have e.g. a workflow step get a transport to perform operations (now you have to reimplement those methods instead, if that possibility is implemented).

[giovannipizzi]

Regarding multiple (sub)classes vs. same class with an attribute to indicate the type: I guess it's (up to a certain level) a matter of taste.
In the meeting a few days ago, we converged on a single StashData, with attributes distinguishing the type (and different from RemoteFolderData).
I guess having an attribute makes it easy to support more types without having to define more classes.
The only thing to think about is how different the Python API will be.
If we expect them to be essentially the same, independent of the StashData type, then a single class+attribute to distinguish is OK.
If we want to provide API to act on each of them, and this is different, we should then probably instead go the multiple-subclasses approach? I'm not sure however of what we might want to support.
Definitely, however, even if we support multiple subclasses, I would vote for a single (sub)class for the StashData generated by the custom script installed on the supercomputer (to be still implemented), since we have no control on what the script will do except for the API that was used to invoke it.

Finally, regarding conversion to RemoteFolderData:

• in the case of copy, we can/should provide a simple calcfunction that converts a StashData in mode copy to a RemoteData
• for any other type, I think converting a StashData to a RemoteData should/could be done with new options of the TransferCalcJob

[ramirezfranciscof]

Note that this is not the only complexity that the engine does well that would be difficult to reproduce by custom code. What happens if your call over transport fails, because the connection is interrupted. The engine has again complex functionality in the EBM to automatically retry and pause if the error persists. Recreating this behavior in client scripts is not trivial. I don't think that you can get away from the fact that if you want to perform tasks over transport, you will have to write asynchronous code that for most users will not be trivial. It is for that reason that I don't think we can make the RemoteData have a simple interface to do operations on. At least not when used in automated processes. For manual use in let's say a shell or command line, that would be different. But you wouldn't be able to just use it in your work chains or calculation jobs. I am not sure if all this is clear.

I don't see that what I'm proposing necessarily implies giving up on the engine extra safeguards. For example, I'm thinking about changing:

        try:
            transport.copy(source_filepath, target_filepath)
        except (IOError, ValueError) as exception:
            execlogger.warning(f'failed to stash {source_filepath} to {target_filepath}: {exception}')

For:

        try:
            remote_folder.raw_copy(transport, source_filepath)
        except (SomeErrorType) as exception:
            execlogger.warning(f'failed to stash {source_filepath} to {remote_folder.path}: {exception}')

The raw_copy method does not currently exist but would be like the copy one except it uses the provided transport instead of opening one. This might seem like not much but would for example allow to instead of specifying a string for the kind of stashing it could just pass the class (RemoteFolderData, RemoteZipData, RemoteScriptData, etc.) which could significantly reduce upkeeping and interdependence between the different parts of the code.

One thing that might complicate this, however, is that a real generic copy procedure depends both on the transport and class of the source and the transport and class of the target. For example, I am not sure what kind of transport is being used in the example above (or in the execmanager in general) because it allows to copy a source from the remote machine into a target in the same remote machine: is this ssh? Because I would expect ssh transport copy stuff from the local machine to the remote machine. Now looking at the plugin code, I see that apparently it has methods for both: copy for the first and put for the second. More confusingly though, the local transport also has both these methods? And now I'm starting to get lost on how transports are supposed to work...can you copy files between machines with local transport put?

I think this refers more to issue #4626 does it? As I responded there to one of Leo's comments, I don't think the class should be determined by what type of stashing was used. It should rather distinguish it from a RemoteFolderData in the sense that that one can be readily accessed, whereas the RemoteStashData might not be, because it may be archived, written to tape, etc.

Yeah, sorry; I did see the comment there. I actually started writting my comment there, but since it was a more "bigger picture" and related many aspects, I wasn't sure exactly where to put it and ended up in here (there are a lot of current threads related to the different parts of this project). Also, maybe this part you quote makes more sense in combination with what I'm proposing before, so the class actually contains the logic on how to access the data through its methods.

EDIT: btw sorry for overlapping with @giovannipizzi , I started writing before his messages were sent he-he

[sphuber]

To comment on the discussion above: I think this might be right moment to change the interface of the RemoteData concerning auto-opening transports. I would add a parameter transport to all concerned functions, e.g. is_empty, getfile, listdir, listdir_withattributes, _clean.

I agree that this would definitely be a step in the right direction. These methods should ultimately be moved from the RemoteData to the RemoteFolderData, because the RemoteData will no longer necessarily be a "folder". It won't even be instantiable any more. Have to think of how the deprecation pathway would go.

For backward compatibility, we make it optional for now - if not specified, we open a transport but also raise a deprecation warning. In 2.0, we make the parameter compulsory.

I am afraid that might be too fast. We sort of agreed to no longer deprecate things in 1.6 that will be removed in 2.0.

We instead add a get_transport method to do it (I would make it public, I think it's convenient for the users - this is the reason we were doing it "implicitly"). If possible (but I'm not sure it's easy) we should also check that the transport is "correct", i.e. to the correct machine (but I'm not sure it's easy).

This is indeed a problem and a critical check to perform but I am not sure it is currently possible. The Transport only get's the hostname of the Computer instance. Now it stands to argue that the hostname should uniquely determine the actual computer, however, in AiiDA it is definitely possible to have multiple Computer instances with the same hostname. A RemoteData will however be coupled to only one of those instances. It is currently not possible to determine from the Transport instance which computer that should be. However, I am also not sure if getting the wrong computer in that case could cause problems. They do represent the same physical machine after all. I guess the easiest would be to pass in the Computer instance in the Transport constructor, but this then tightly couples that class to an AiiDA ORM class, which is subideal as well.

Reason: while I agree that this only shifts the problem, it shifts it in the right direction: now, a conscious user cannot avoid to open a lot of connections. This will open up the possibility in the future to have e.g. a workflow step get a transport to perform operations (now you have to reimplement those methods instead, if that possibility is implemented).

While this is true, this does not solve the problem of connection problems. If you add this code into your workflow, even though it may use controlled transports to not overflow the machine, the transport operation may fail, which will except and which will except the entire workchain, toppling everything upstream as well. I think one would have to write this code such that it is wrapped in an EBM-like retry. This is possible, it would require the user to start writing asynchronous code.

@ramirezfranciscof I see now that you are proposing to move the logic for the actual operations as much as possible to within the data types. I am not opposed to this per sé and might be good in certain cases. Would have to look more into this. However, as explained above, this would still not solve the main problems unless they are actually called by the engine in the calcjobs. Users calling this code themselves in their workchains is problematic. Now it would be great if we could fix this in a generic way, but this will certainly be far from trivial.

The raw_copy method does not currently exist but would be like the copy one except it uses the provided transport instead of opening one. This might seem like not much but would for example allow to instead of specifying a string for the kind of stashing it could just pass the class (RemoteFolderData, RemoteZipData, RemoteScriptData, etc.)

What do you mean with "instead of specifying a string"? You mean the input in the stash metadata options to indicate the type of stashing that needs to be performed? I don't see what passing a class would solve. Since this will have to be stored as an attribute, the class will have to be serialized, which essentially means it is transformed into a string anyway.

For example, I am not sure what kind of transport is being used in the example above (or in the execmanager in general) because it allows to copy a source from the remote machine into a target in the same remote machine: is this ssh?

It is whatever transport the computer is configured to require, and the computer being the one to which that calculation job was submitted. If it was submitted to a computer with local transport, it will be a local transport instance.

Because I would expect ssh transport copy stuff from the local machine to the remote machine.

You are confusing the fact here that the "remote" machine is simply the computer to which the calcjob was submitted. This might just as well be the localhost, in which case it would most likely use local transport but not necessarily. You can also configure to connect over ssh to your localhost. Again, the "remote" computer in AiiDA is not always "really remote". Important not to forget that.

More confusingly though, the local transport also has both these methods? And now I'm starting to get lost on how transports are supposed to work...can you copy files between machines with local transport put?

The Transport is simply an abstract interface to copy and retrieve files and folders from one computer to the other. AiiDA is always running on the "one" computer, and it is connecting to the "other". The SshTransport facilitiates this interaction if the "other" computer is (only) reachable over an SSH connection. The LocalTransport implements the operations if the "other" computer is the same as the "one" computer. That's all.

[ramirezfranciscof]

It is whatever transport the computer is configured to require, and the computer being the one to which that calculation job was submitted. If it was submitted to a computer with local transport, it will be a local transport instance.

You are confusing the fact here that the "remote" machine is simply the computer to which the calcjob was submitted. This might just as well be the localhost, in which case it would most likely use local transport but not necessarily. You can also configure to connect over ssh to your localhost. Again, the "remote" computer in AiiDA is not always "really remote". Important not to forget that.

The Transport is simply an abstract interface to copy and retrieve files and folders from one computer to the other. AiiDA is always running on the "one" computer, and it is connecting to the "other". The SshTransport facilitiates this interaction if the "other" computer is (only) reachable over an SSH connection. The LocalTransport implements the operations if the "other" computer is the same as the "one" computer. That's all.

Ah, I see. Yes, I do know the remote computer can actually be your local one (I have run in localhost many times, haha), but I thought it might use the same ssh transport for that (I mean, you can ssh to your own computer), but it is true that it does not have the information to do that is the computer transport is set to local (i.e. passwords, key, etc that goes into the computer config). It makes sense in retrospect.

Also thanks for the explanation on the transports; so basically for local transport the put and copy methods will do the same thing.

@ramirezfranciscof I see now that you are proposing to move the logic for the actual operations as much as possible to within the data types. I am not opposed to this per sé and might be good in certain cases. Would have to look more into this. However, as explained above, this would still not solve the main problems unless they are actually called by the engine in the calcjobs. Users calling this code themselves in their workchains is problematic. Now it would be great if we could fix this in a generic way, but this will certainly be far from trivial.

I see what you mean; I guess @giovannipizzi was thinking more exclusively on allowing the user the "performant" way of transporting, not necessarily the whole retry on error infrastructure. They would anyways be able to do some manual retry, no? Like, for example:

# very pseudo pseudo-code
tries = 0
while error and tries < 5:
    tries =+ 1
    error = None
    try:
        folder_node.copy(transport,file)
    except RaisedError as er:
        error = er

The "general solution" problem seems interesting though. I clearly don't know enough of how this part work to propose a very specific solution, but would it be possible to automatically convert the steps of the workchain into async routines? And then maybe when you execute them if they catch a RetryError type (which would have to be manually raised by the workchain designer) it would mean they have to retry this step?

What do you mean with "instead of specifying a string"? You mean the input in the stash metadata options to indicate the type of stashing that needs to be performed? I don't see what passing a class would solve. Since this will have to be stored as an attribute, the class will have to be serialized, which essentially means it is transformed into a string anyway.

Well, sure you serialize to store but you already have rules to serialize classes, no? What I mean is now you don't need to create and maintain an "extra" equivalence relationship with the enum of possible stashing methods (you have the def of the enum on one part of the code, but then you have to "if ==" through it to decide which code to execute). The classes available are the possible stashing methods (and the code of what to do is in the class and passing the class is giving the instructions, there is only one place where the option is fully specified).

If you will, it would be the same if your enum was instead a dict with translator = {option_name: code_to_execute} and you just execute translator[option_passed] (if such a thing was possible).

Since now there is only one option possible, perhaps I'm not imagining correctly how you were thinking to implement the selector of what to do when we extended the functionality.

[sphuber]

I see what you mean; I guess @giovannipizzi was thinking more exclusively on allowing the user the "performant" way of transporting, not necessarily the whole retry on error infrastructure. They would anyways be able to do some manual retry, no?

Yes, they would be able to do this. Still that doesn't solve the problem where people will put this in workflows, which is where it will break down. You will want a "sleep" in your retry to give a grace period, and if you don't do this async, you block the entire thread. Then of course you still want to "pause" and not except after max retries which again is not trivial.

The "general solution" problem seems interesting though. I clearly don't know enough of how this part work to propose a very specific solution, but would it be possible to automatically convert the steps of the workchain into async routines? And then maybe when you execute them if they catch a RetryError type (which would have to be manually raised by the workchain designer) it would mean they have to retry this step?

As far as I am aware, it is not possible to convert sync code to async code on-the-fly. It is not just slapping async in front of the method definition, you will also have to add await in parts of the code where you call other coroutines. The only solution is to require users to write async code to begin with. But I am not sure if we can allow to write "mixed" code, i.e. have only some of the outline steps in a workchain be async.

Since now there is only one option possible, perhaps I'm not imagining correctly how you were thinking to implement the selector of what to do when we extended the functionality.

No, that is more or less how I was imagining implementing it and extending it once more modes become available. I don't really see how this is such a problem though. Sure you can remove the if/else and just rely on passing the class and passing the single responsible method. To me that doesn't change that much, but it does add complexity elsewhere. We do not yet support serialization of arbitrary classes in attributes. Additionally, your approach assumes that the interface of this stash method on those RemoteStashData subclasses can have exactly the same interface. For normal copy and tar, maybe yes, but also for the custom remote script one? In the end, you may end up doing the same if/else, but on the class instead of the mode enum. Again, I don't see this as the main problem in this whole discussion.

TLDR: SUMMARY OF THREAD to summarize this very long discussion in how I see things:

• The question and difference of opinion seems to be whether the RemoteData classes should function mostly as a data representer or as an operator. That is to say, should they merely store and reflect what data they contain, or should they also actually be functional classes to operate on that data.
• From how classes are typically used in Python, it seems intuitive to make them operators and therefore implement functionality to manipulate their data. This seems to be what @ramirezfranciscof is suggesting.
• On the contrary, I am not convinced that this is a wise design. The RemoteData needs a lot more information that is not really part of it, to perform these operations on data. It depends on the computer that it is one (which, technically can be retrieved from the RemoteData) and a transport to connect to that computer. The RemoteData is really just a "symbolic link". Any instance of it, doesn't really have access to its data, like an instance of a class typically has. It just has a reference which allows to address it, but only when it gets external resources that are required. It is therefore that I think we should treat these objects more as simple containers of a reference to data. The operator should be external to it and use it just to get the correct "address" and format of the data they reference.
• The problem that I see with treating RemoteData as operator classes, is shown when it is used in automated code. It needs to have the transport passed in (doable, but already moves in the direction of the RemoteData no longer being the operator) and it does not have the capability itself to automatically recover from transient problems. Also this has to be done from the outside.
• Now of course, these objections are not as much of a problem when used interactively, but AiiDA is mostly an automated infrastructure and not primarily designed to be an interactive tool.
• That being said, I see the argument that it should be usable as both. My final observation is just that if you give the RemoteData class "operating" abilities, then people will use it and they will put it in workflows. We can of course document that they shouldn't do this, but we will then still have to implement the operating code in the engine.

FINAL QUESTION Does any of this discussion have any impact on the fundamental question of whether we should have the stashing functionality on a CalcJob at all? Or are we merely talking on how the various subclasses of RemoteData should be organized and what exact functionality they should possess?

[sphuber]

@chrisjsewell my pre-commit and docs checks started failing here ever since you released plumpy==0.18.4. Could you have a look and see whether you know why and if this is something we have to fix in aiida-core or in plumpy?

[chrisjsewell]

@chrisjsewell my pre-commit and docs checks started failing here ever since you released plumpy==0.18.4. Could you have a look and see whether you know why and if this is something we have to fix in aiida-core or in plumpy?

well that will be motivation for you to have a look at #4669 then 😉

[chrisjsewell]

as I mention there, I'm not quite sure why mypy is not recognizing e.g. plumpy.Wait, and you need to have the full path plumpy.process_spec.Wait

[mbercx]

Thanks @sphuber! I gave the code a first pass and left some comments. I'll do a bit of field testing and if I don't run into any issues write a first draft for the documentation.

[giovannipizzi]

Create the new remote.stash.folder: RemoteStashFolderData to be used for the copy mode.

OK

Change the current remote.stash: RemoteStashData entry point to be a non-instantiable base class

OK

I would leave the stash based entry points under the remote. hierarchy. Any downsides beside the query problem marked by Giovanni?

I see your comment on array vs array.kpoints (even if in that case it's really just an ArrayData node with some additional python methods, so really a subclass also in the python sense, here is a different implementation). On the other hand I agree that it's more intuitive if they live all inside remote. I don't see other downsides currently (and I really see limited need to query only for exactly RemoteData + only one subclass of stash) so OK, for me, to go ahead like this, also not to block this forever, and I think this is the easiest approach.
[EDIT: thinking better to it, one issue is people searching for all output RemoteData nodes of a ProcessNode and now (by default, with subclassing = True) finding more than one. But maybe it's OK - there is the word 'remote' in the RemoteStash*Data class name(s), and if they have some stash data, it means they explicitly asked to stash].

It would be great, though, if @mbercx (and @ramirezfranciscof ?) could confirm that there are no other issues, and (especially for @mbercx who's been keeping the design notes updated) could confirm that we're not missing one of the points we've been discussing, before you @sphuber do the work.

[mbercx]

I don't know the history of that naughty KpointsData class, but I also don't see any downsides to having multiple classes for different types of stashing modes. It just offers us more flexibility in the future, and allows the user to query for a specific type of stashed data.

I've gone through the notes again, and I don't see any more issues with the currently proposed implementation. Just wanted to highlight again that we should also consider the unstashing of the data, via e.g. the TransferCalcJob. I can't think of any problems right now, but maybe you can still think of something that I've missed.

[sphuber]

@ramirezfranciscof and @mbercx this is ready for review. I kept the final commit separate to easily see the changes since last review, but it should be squashed upon merge.

[mbercx]

Thanks @sphuber! Changes look good, but I think you still need to add _storable = True to the RemoteStashFolderData class?

I'll open a separate PR for the documentation. I think I'll add a subsection to the "How to run external codes" section.

[sphuber]

You are absolutely right. I have fixed it and added a unit test for it. I am hoping that this is what caused the daemon tests to hang.

[sphuber]

Alright @mbercx seems like the halting daemon tests were indeed just due to the unstorableness. That is fixed now and all tests pass. @ramirezfranciscof this is ready for review should you have the time.

[chrisjsewell]

@mbercx, @ramirezfranciscof can you give a timeline on when you can review this?

[mbercx]

@mbercx, @ramirezfranciscof can you give a timeline on when you can review this?

This is good to go for me, the only issue I still saw was the storability one. I think we're just waiting for @ramirezfranciscof's stamp of approval?

[ramirezfranciscof]

Yeah, sorry, I'm half way through the thing but I have another big pressing priority I need to finish right now and is consuming a lot of my attention 😅. If you are tired of waiting I would understand, I can send the couple of comments I had (very minor, nothing critical so far I think) and just move on. Otherwise I could try to finish this tomorrow maybe (it really does depend if I manage to break through some specific obstacle I'm dealing with).

[ramirezfranciscof]

Apologies for taking a long time to see this, but all great! I just have a couple of comments and questions to understand this better (since this was already approved I'll just leave this as a comment).

[ramirezfranciscof]

Should we maybe also test the behaviour for when the target_base path doesn't exist?

I was also going to say to check what happens when a non available stash_mode is provided, but then I saw at the end you did this in the tests/orm/data/test_remote_stash.py. I'm still wondering if it makes sense to check how the daemon handles that error too. I guess this is also related to what Giovanni mentions re- the codecov complaints, so perhaps you already thought about it and discarded the idea. Up to you.

[sphuber]

I was also going to say to check what happens when a non available stash_mode is provided, but then I saw at the end you did this in the tests/orm/data/test_remote_stash.py. I'm still wondering if it makes sense to check how the daemon handles that error too. I guess this is also related to what Giovanni mentions re- the codecov complaints, so perhaps you already thought about it and discarded the idea. Up to you.

It is a good point, but tests/orm/data/test_remote_stash.py doesn't test what you want it to test. It merely checks that the node implements the checks for valid values. What you mean is to check that the CalcJob process also validates properly. This is done however. In tests/engine/test_calc_job.py all the way at the bottom I test the validator for the stash options, which ensures that the process won't even be created when an invalid stash mode is defined.

Should we maybe also test the behaviour for when the target_base path doesn't exist?

Could do. Would it be ok to only check that, e.g., by simply deleting the tmpdir that is generated before launching it? If I have to launch an additional calculation just for the other case as well, these tests will get out of control. This is the downside of integration tests. You are running so much code just to test one minor variation and target a single line.

[ramirezfranciscof]

It is a good point, but tests/orm/data/test_remote_stash.py doesn't test what you want it to test. It merely checks that the node implements the checks for valid values. What you mean is to check that the CalcJob process also validates properly. This is done however. In tests/engine/test_calc_job.py all the way at the bottom I test the validator for the stash options, which ensures that the process won't even be created when an invalid stash mode is defined.

Ah, good point, you are right.

Could do. Would it be ok to only check that, e.g., by simply deleting the tmpdir that is generated before launching it? If I have to launch an additional calculation just for the other case as well, these tests will get out of control. This is the downside of integration tests. You are running so much code just to test one minor variation and target a single line.

Uhm, ok I see; yes, if it just re-creates it then it should be ok. Now, not for this PR, but maybe for a later discussion I wonder if we want to allow this path creation in a remote cluster where trying to just create folders in arbitrary locations may lead to trouble. Does the aiida working directory for remotes also create the full path if there are missing dirs? I don't know, maybe I'm being overly cautious...

[ramirezfranciscof]

I'm not entirely sure why of this change. Maybe you already commented on this on one of the meetings we had and I don't remember, but I couldn't find anything in the text of the PR. Why did you add the DONE state? And why did you change it here instead of PARSING? What is the purpose of these checks here if the states are supposed to be called in order, just a sanity check?

[chrisjsewell]

What is the purpose of these checks here if the states are supposed to be called in order, just a sanity check?

No @ramirezfranciscof this is if e.g. the daemon was stopped/restarted during the transport tasks, we don't want to repeat a task that has already been finished.

but yes @sphuber it appears here that you have simply replaced PARSING with DONE?
Is this the case and what was the reason?
If it is a swap then surely PARSING should be removed from CalcJobState and also the places in the tests that use PARSING should also be replaced with DONE.
Technically this would also warrant a database migration, which brings into question if it is really worth it 😬

[sphuber]

This change was necessary for one of the previous designs where I think the stashing was performed after the parsing and so a new state was necessary. However, in the current design, the stashing (if needed) is done before retrieving and the parsing then follows retrieving as it used to. So indeed now the change is merely replacing PARSING with DONE so I can revert it. Note that this state attribute is merely for internal purposes and is even removed after the process is terminated, so a migration wouldn't have been necessary.

[chrisjsewell]

cheers

Note that this state attribute is merely for internal purposes and is even removed after the process is terminated, so a migration wouldn't have been necessary.

well I meant in the (very rare case) a migration occurred whilst you still had pending calculations in the PARSING state, but yeh a technicality to be sure

[ramirezfranciscof]

What kind of exception is this catching? Can't we re-raise something more informative?

[chrisjsewell]

its catching what it says its catching 😜 an interruption, like pausing or killing, so no

[ramirezfranciscof]

Right, but my underlying question is why would you catch the exception to just raise with nothing? Why not just leave plumpy.process_states.Interruption be the raising error?

[chrisjsewell]

because otherwise it would be caught in the exception below

[sphuber]

Just in case this what your question was trying to address: if you catch and then simply call raise again, it will raise the original exception. Why is this useful? Because this way you can treat certain exceptions different from others. In this case we want exceptions to be treated a certain way, except the Interruption, which should just be bubbled up.

[ramirezfranciscof]

if you catch and then simply call raise again, it will raise the original exception.

Ahh ok. Yeah, sorry, I did not know this, I thought you had to explicitly provide the exception to raise.

[chrisjsewell]

from a quick skim, generally fine cheers, but still a few changes to make

[chrisjsewell]

What is the purpose of these checks here if the states are supposed to be called in order, just a sanity check?

No @ramirezfranciscof this is if e.g. the daemon was stopped/restarted during the transport tasks, we don't want to repeat a task that has already been finished.

but yes @sphuber it appears here that you have simply replaced PARSING with DONE?
Is this the case and what was the reason?
If it is a swap then surely PARSING should be removed from CalcJobState and also the places in the tests that use PARSING should also be replaced with DONE.
Technically this would also warrant a database migration, which brings into question if it is really worth it 😬

[sphuber]

Alright gents, thanks for the review. Should have addressed all of them. If it is ok to be approved, please let me merge myself as I need to adapt the commit message a bit.

[chrisjsewell]

good job 👌

[ramirezfranciscof]

Great work, thanks!