Fix behavior of RemoteData.getfile()

[Crivella]

The :return: value in the documentation was not correct.

[ltalirz]

Thanks @Crivella and welcome to the aiida-core contributors!

I have just one small suggestion - to adapt also the first line of the docstring.

[ltalirz]

Indeed, this seems to be what it is doing...
@giovannipizzi Is there a good reason for doing that (e.g. potentially returning the names of files that have nothing to do with the one that was just transferred?)

[ltalirz]

thanks @Crivella

[ltalirz]

Just coming back to this - it seems I didn't look at this properly, and you are actually pointing out a bug here, right?

The getfile is currently not doing what its name (or the first comment line) are advertising and needs to be fixed.

It looks to me like one needs to add a return before t.getfile(full_path, destpath):

aiida-core/aiida/orm/nodes/data/remote.py

Lines 67 to 79 in 0644e7d

	with t:
	try:
	full_path = os.path.join(self.get_remote_path(), relpath)
	t.getfile(full_path, destpath)
	except IOError as e:
	if e.errno == 2: # file not existing
	raise IOError('The required remote file {} on {} does not exist or has been deleted.'.format(
	full_path, self.computer.name
	))
	else:
	raise
	return t.listdir()

And if the file is not found, the function already raises an IOError, so you should not need to return anything else.

Do you want to fix this or should I do it myself?

[ltalirz]

thanks @Crivella !
This looks like the correct way to fix this.

[Crivella]

Actually as of know this method would have the double behaviour of copying the file to the local machine and returning its content as a string.

I'm not sure if it would be better to split this two into separate methods.

[ltalirz]

Thanks for pointing this out.

It turns out that copying the file is actually the only thing it does

aiida-core/aiida/transports/plugins/local.py

Lines 465 to 492 in 66b6f6a

	def getfile(self, remotepath, localpath, *args, **kwargs):
	"""
	Copies a file recursively from 'remote' remotepath to
	'local' localpath.
	:param remotepath: path to local file
	:param localpath: absolute path to remote file
	:param overwrite: if True overwrites localpath.
	Default = False
	:raise IOError if 'remote' remotepath is not valid or not found
	:raise ValueError: if 'local' localpath is not valid
	:raise OSError: if unintentionally overwriting
	"""
	overwrite = kwargs.get('overwrite', args[0] if args else True)
	if not localpath:
	raise ValueError('Input localpath to get function must be a non empty string')
	if not remotepath:
	raise IOError('Input remotepath to get function must be a non empty string')
	the_source = os.path.join(self.curdir, remotepath)
	if not os.path.exists(the_source):
	raise IOError('Source not found')
	if not os.path.isabs(localpath):
	raise ValueError('Destination must be an absolute path')
	if os.path.exists(localpath) and not overwrite:
	raise OSError('Destination already exists: not overwriting it')
	shutil.copyfile(the_source, localpath)

i.e. t.getfile actually does not return anything.

Since the method of RemoteData is called getfile as well, I would suggest that its behavior should actually be the same as the getfile of the transport - i.e. not return anything.
Anyhow, let's wait for @giovannipizzi to comment on this , whom you tagged in the PR originally.

Sorry for these confusing interaction from my side (I never actually used this function and should really have paid closer attention).
That none of these changes caused any tests to fail also seems to indicate that this function is currently untested.

[giovannipizzi]

Hi all.
Indeed, these were utility functions I added at some point. I think the return value is a copy-paste error, so OK to not return anything.
So for this PR I would just remove the return value (and, ideally, add a test for this and for listdir).

More generally, let's discuss in #1857 if we want to keep these functions or how to make it clear that they open an transport and you do not want to do this in a workflow (see also #3478 )

[ltalirz]

@giovannipizzi
Since I think we agree that the return value of this function was really a bug, I think it's fine to change without a deprecation warning.

Please review.

[codecov]

Codecov Report

Merging #3742 into develop will decrease coverage by 0.00%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #3742      +/-   ##
===========================================
- Coverage    77.17%   77.17%   -0.01%     
===========================================
  Files          457      457              
  Lines        33775    33774       -1     
===========================================
- Hits         26066    26065       -1     
  Misses        7709     7709              

Flag	Coverage Δ
#django	69.21% <ø> (-0.01%)	⬇️
#sqlalchemy	70.03% <ø> (-0.01%)	⬇️

Impacted Files	Coverage Δ
aiida/orm/nodes/data/remote.py	59.37% <ø> (-0.42%)	⬇️

---

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 035a9da...4801e5e. Read the comment docs.

[giovannipizzi]

Thanks a lot @Crivella and @ltalirz!
@sphuber can I update this branch and merge? Or are you working on merging? (Ok I you want to take care of this.

[sphuber]

Thanks guys, I will merge another PR first that is already good to go and will then update this and merge it

[sphuber]

Ok, the other PR has problems with the tests, so I will merge this first after all

Changes addressed