ORM: Add get_size_on_disk method to RemoteData

[GeigerJ2]

Required for PR #6578.

By default, the get_size_on_disk method calls the method _get_size_on_disk_du that uses du to obtain the total directory size in bytes. If the call to du fails for whatever reason, recursive stat is being used, though, that is discouraged as stat returns the apparent size of files, not the actual disk usage.

I further extended the existing tests for RemoteData to use both, LocalTransport, as well as SshTransport, and test for the functionality added in this PR.

Pinging also @npaulish, as she's currently working on retrieving RemoteData objects.

[codecov]

Codecov Report

Attention: Patch coverage is 82.60870% with 16 lines in your changes missing coverage. Please review.

Project coverage is 77.94%. Comparing base (c532b34) to head (9c3d2ba).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/aiida/orm/nodes/data/remote/base.py	88.24%	8 Missing ⚠️
src/aiida/cmdline/commands/cmd_data/cmd_remote.py	50.00%	7 Missing ⚠️
src/aiida/common/utils.py	90.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6584      +/-   ##
==========================================
+ Coverage   77.92%   77.94%   +0.02%     
==========================================
  Files         563      563              
  Lines       41671    41761      +90     
==========================================
+ Hits        32467    32545      +78     
- Misses       9204     9216      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[giovannipizzi]

This is maybe machine-dependent, but rather than going via our API (that is more robust, but definitely going to be slower, I think) have a first "fast" option just running du -s and parsing the output (but careful about units! E.g. it uses "blocks", on some systems it's 512 or 2048bytes!! And if it fails, fall back to your solution?

[GeigerJ2]

Note to self to run du via exec_command_wait method from transport.

[khsrali]

Thanks @GeigerJ2 please go through more pains that I imposed 😈

[GeigerJ2]

Not sure why my replies via the files GH tab appear as a review, but whatever... ^^

Thanks for the review @khsrali. I implemented most of your proposed changes!

As to our in-person discussion if du or lstat is preferred, I think none of the two is ideal... lstat giving the actual byte-sized content, which is neat and all, but won't correspond to the disk space that will actually be occupied locally by a file, due to the use of blocks for the file system. And du giving the actual occupied disk space on the remote, which, however, might be different from the local file system (due to having a different file system on the local machine, different formatting, different block size, etc.). Hence, the big difference in the file size check in the test. For real-world use cases, with more and larger files, the difference is likely much smaller, and won't matter too much, I think. I'll add a test for a larger file, as well as modify the message that the given size is just an estimate, so that user are aware they should take the value with a grain of salt.

Maybe also @agoscinski with his actual computer science background can weigh in 🫶

[khsrali]

Thanks a lot @GeigerJ2! Just a few minor comments..

[agoscinski]

Yes seems like we should split the function into two or provide optional arguments. These two concepts seem to be distinguished by the terms "disk usage" and "apparent size" (see https://stackoverflow.com/a/569485). I think the output of du --apparent-size is the same as with lstat, so you do not need to use lstat

[GeigerJ2]

Thanks for the review, @agoscinski! I'm currently still working on this, will ping you once it's again ready for review.

I think the output of du --apparent-size is the same as with lstat, so you do not need to use lstat

The reason I'm providing lstat as a fallback option is if du is not available (e.g., MacOS, as you mentioned, didn't know that ^^), or if exec_command_wait isn't available, which will be the case for FirecREST.

[GeigerJ2]

OK, this should be ready for a final review, @agoscinski and @khsrali. Also pinging, @mikibonacci, if you want to provide some feedback on the CLI/API for use in AiiDAlab?

[khsrali]

Thanks @GeigerJ2 ,
Please consider my review, I haven't checked the tests thoroughly. I leave that to @agoscinski

[GeigerJ2]

Thanks again for the review, @khsrali, I implemented your proposed changes.

[khsrali]

Thanks @GeigerJ2 , all good. Just two comments:

1. I find it confusing that you call it stat while it's actually doing a lstat.
2. Maybe consider to raise ValueError instead of Notimplemented.

[khsrali]

I think it makes more sense for this to be a ValueError, otherwise might be confusing when there's a typo, e.g. 'state' -> NotImplemented (?)

if you have any specific method in mind (like what actually? 🤔 ) that is legit but not implemented, you could single them out and raise NotImplemented.
But in generic cases I'd raise ValueError which is more clear.

Suggested change

	raise NotImplementedError(
	f'Specified method `{method}` for evaluating the size on disk not implemented.'
	)
	raise ValueError(
	f'Specified method `{method}` is not an valid input. Please choose either 'du' or 'stat'.'
	)

[GeigerJ2]

Ah, I see your point. Yeah, the idea was indeed to hint at the possibility of more options to evaluate the disk usage to be implemented in the future. Though, now that I think of it, I wouldn't know what that would be, honestly ^^ so I agree it's better to change it to a ValueError to capture typos, so I modified it as you suggested. Thanks!

[khsrali]

Why did you change to stat, in the end?
What you are doing here is actually lstat.
Links are not followed.

[GeigerJ2]

I changed it because I think people are more familiar with stat, as it is also a Linux command line utility, whereas lstat is not. Also, lstat is just the same as os.stat(follow_symlinks=False), and I don't think the implementation detail should make the user-facing option less understandable. Lastly, I also think it's self-explanatory that, to get the size of a directory on disk, symlinks that point to files/directories that life somewhere else, should not be considered. That being said, we could also think about exposing the follow_symlinks option to the user, though that would also mean updating the implementation of listdir_withattributes of the Transport, so I wouldn't do that now, this PR has been dragging on for way too long anyway :D

[GeigerJ2]

Thanks again for the review, @khsrali. I wrote down my reasoning for point 1 in my response to your comment in the code, and implemented point 2. Once CI passes here (hopefully), I'll squash-merge.