Skip to content
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

Allow verdi calcjob outputcat work with binary files #4077

Merged
merged 6 commits into from
May 20, 2020
Merged

Allow verdi calcjob outputcat work with binary files #4077

merged 6 commits into from
May 20, 2020

Conversation

zhubonan
Copy link
Contributor

@zhubonan zhubonan commented May 17, 2020

Fixes #4086

This PR allows verdi calcjob outputcat to work with binary files such as those compressed by gzip. Previously, only plain text files can be printed because of the missing parameter mode in get_object_content, which defaults to r.

Edit:

Now just open the file as it is in bytes, and use shutil.copyfileobj to do the copying. This function copies the content in chunks and avoids high memory usage. The command should now work with both text files and binary data.

@codecov
Copy link

codecov bot commented May 17, 2020

Codecov Report

Merging #4077 into develop will increase coverage by 0.02%.
The diff coverage is 62.50%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4077      +/-   ##
===========================================
+ Coverage    78.76%   78.78%   +0.02%     
===========================================
  Files          463      463              
  Lines        34402    34414      +12     
===========================================
+ Hits         27095    27108      +13     
+ Misses        7307     7306       -1     
Flag Coverage Δ
#django 70.69% <62.50%> (+0.02%) ⬆️
#sqlalchemy 71.57% <62.50%> (+0.02%) ⬆️
Impacted Files Coverage Δ
aiida/cmdline/commands/cmd_calcjob.py 68.80% <62.50%> (+0.38%) ⬆️
aiida/cmdline/commands/cmd_node.py 82.57% <62.50%> (+0.90%) ⬆️
aiida/transports/plugins/local.py 80.21% <0.00%> (-0.25%) ⬇️
aiida/engine/daemon/client.py 73.72% <0.00%> (+1.15%) ⬆️

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 51ea47d...a80b85d. Read the comment docs.

Now just open the file as it is in bytes. Use copyfileobj to do the
copying in chunks and avoid high memory usage.
Copy link
Member

@giovannipizzi giovannipizzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments

aiida/cmdline/commands/cmd_calcjob.py Outdated Show resolved Hide resolved
aiida/cmdline/commands/cmd_calcjob.py Outdated Show resolved Hide resolved
@giovannipizzi
Copy link
Member

BTW, since you are at it, probably you might want to:

  • fix also inputcat, and the cat command of verdi node repo, if it has the same issue?
  • add some tests?

@zhubonan
Copy link
Contributor Author

@giovannipizzi Done! I have also added the tests.

@sphuber
Copy link
Contributor

sphuber commented May 19, 2020

Looking good @zhubonan , thanks a lot. There is also verdi node repo cat that suffers from the same problem. Would you be willing to add your fix there as well?

@zhubonan
Copy link
Contributor Author

@sphuber i thinks that has been fixed as well in the latest pushes (please do double check). It is really to merge now.

@sphuber sphuber self-requested a review May 20, 2020 06:48
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @zhubonan

@sphuber sphuber merged commit b86425f into aiidateam:develop May 20, 2020
@sphuber sphuber deleted the pr-outputcat-allow-b branch May 20, 2020 06:52
@sphuber sphuber modified the milestone: v1.2.2 May 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The verdi commands that cat file content break for binary files
3 participants