Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add
get_size_on_disk
method toRemoteData
#6584base: main
Are you sure you want to change the base?
Add
get_size_on_disk
method toRemoteData
#6584Changes from all commits
1aeea2f
d7af702
10e0a62
d08be75
26abd5a
4fcb1ba
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this whole function, doesn't really need to be separated I feel.. It's basically only executes a command, and you don't re-use it, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found the design to have just one top-level method
get_size_on_disk
nice, which tries to call the two private methods, in terms of separation of concerns.Though, it is true that this pollutes the API of
RemoteData
somewhat... So I'm also fine of either moving the private methods to someutils
module, or merging them. Maybe @unkcpz can comment on good coding practices here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I understand, sorry 😬
Since it's not really re-usable by other method, I'd vote for merging it, and avoid over-fictionalizing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I found the term recursively confusing.. (thought the function does
yield
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that already returns the human readable one, why not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least for testing and validation, I think returning the bytes is more convenient. I'd convert it to the human-readable format only at the last step, when printing it to the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one more concern, is the
aiida-firecrest
plugin..they don't support command execution, so it's better we avoid adding more calls to
exec_command_wait
in the code base, wherever it's not absolutely crucial...I mean it adds maintenance overheads.. in future somebody will open an issue and PR to change this..
Also your nice
_get_size_on_disk_lstat
function is already addressing this functionality, sodu
doesn't seem super crucialThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I can add a check on the transport-type before, to make sure it stays compatible with FirecREST in the future. Though, I wouldn't remove the convenient and preferred implementation for now in anticipation of FirecREST eventually becoming the required transport mechanism for CSCS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, please! Makes sense to add a
except NotImplementedError
on line 232, in caseexec_command_wait
is not implemented.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the same:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙉
aiida_computer_ssh
should be compared withaiida_computer_local
, which in that sense they are similar.. the issue is we don't have "aiida_ssh
" that would return the actual computer instance, lol 🙉There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, exactly. We could add a fixture that actually calls the factory and returns the
Computer
instance, though I'd rather call itaiida_localhost_ssh
, and don't see the need for it right now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yews, so maybe please fix this comment, which I found confusing: