passing unstored data node surfaces programming error in aiida-core

[ltalirz]

as discovered by @danieleongari, you can get an error message like this

 File "/home/daniele/anaconda3/envs/aiida1/lib/python3.6/site-packages/plumpy/process_states.py", line 220, in execute
    result = self.run_fn(*self.args, **self.kwargs)
  File "/home/daniele/aiida1/aiida_core/aiida/engine/processes/calcjobs/calcjob.py", line 261, in run
    upload_calculation(self.node, transport, calc_info, script_filename, dry_run=True)
  File "/home/daniele/aiida1/aiida_core/aiida/engine/daemon/execmanager.py", line 176, in upload_calculation
    handle.write(data_node.get_object_content(filename, mode='rb'))
UnboundLocalError: local variable 'data_node' referenced before assignment

when passing an unstored node as input to a builder like this:
https://github.com/danieleongari/aiida-raspa/blob/6ebe6117b6d28bbe655b4d33309c550b4ad35ee2/examples/simple_calculations/test_ff_files.py#L74-L79

In my view, there are two issues here:

1. There's a minor programming error, where this should be under finally:
   

   aiida-core/aiida/engine/daemon/execmanager.py

   Lines 175 to 179 in cc94a1e

   	with NamedTemporaryFile(mode='wb+') as handle:
   	handle.write(data_node.get_object_content(filename, mode='rb'))
   	handle.flush()
   	handle.seek(0)
   	transport.put(handle.name, target)

2. For some reason, the node is not stored automatically when passed to the Process via the builder - to figure out why

[ltalirz]

@sphuber Do you happen to know the reason for 2.?

[sphuber]

Most likely because he ran a dry_run with store_provenance=False.

Regarding the bug itself, it should not be in the finally but the else. If you were to put it in the finally it would also get there in case of the exception, i.e. the node could not be loaded. However, we might want to change this code, because this is essentially broken when running without provenance. We don't give very strong guarantees yet about running without provenance, but something could be said that it should at least work in combination with dry_run, in which case we recommend setting store_provenance=False. Essentially we would have to pass in the process inputs into the upload_calculation call, which should use them, instead of loading it by UUID to get the relevant data nodes used in the local_copy_list specifications. However, since the input space can be nested, finding the corresponding node will have to be done recursively. Think it can be done, but would be a bit ugly.

[ltalirz]

Regarding the bug itself, it should not be in the finally but the else

Hehe thanks ;-) turns out I had not understood what finally actually does.
I checked it and now I know more :-)

it should at least work in combination with dry_run, in which case we recommend setting store_provenance=False

I would agree...