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

CLI: progress bar added for verdi storage maintain with disk_object_store backend #6562

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

khsrali
Copy link
Contributor

@khsrali khsrali commented Sep 3, 2024

This is the complementary PR for aiidateam/disk-objectstore#171

Now one can easily test the changes here, as @giovannipizzi and @unkcpz requested.

Note

This PR should only merge after the one mentioned above, for this reason I marked this one as draft.
Also I should remember to tag and update the dependency,

Note 2

Don't worry about test failing, it's only because it depends on a released version of disk-objectstore
Ideally for testing, you can pull the PR and install with pip install -e .

@unkcpz
Copy link
Member

unkcpz commented Sep 4, 2024

Thanks @khsrali. The change makes sense, but I think it need a flag for verdi storage maintain to trigger this (or better as the default flag). Otherwise change the log level to show the progress is not very convenient.

@khsrali
Copy link
Contributor Author

khsrali commented Sep 4, 2024

I think it need a flag for verdi storage maintain to trigger this (or better as the default flag). Otherwise change the log level to show the progress is not very convenient.

I tried to stay with same convention of verdi see for instance:
verdi archive import, verdi archive migrate, and verdi archive create in cmd_archive.py they do the same.
The progress bar is shown in the default log level, which is good for baby users.
And of course if an advanced users is so annoyed by it, they can simply set it off in the log level.

@unkcpz
Copy link
Member

unkcpz commented Sep 4, 2024

Okay, then can you move the change from backend to cmd module as in cmd_archive.py

@khsrali
Copy link
Contributor Author

khsrali commented Sep 4, 2024

Okay, then can you move the change from backend to cmd module as in cmd_archive.py

ok, pls check now.

@khsrali
Copy link
Contributor Author

khsrali commented Sep 4, 2024

Actually, I don't like it now, spreading everything in separate files really reduces the readability of the code.
Note the context manager should remain in the backend, as that's the right use of it.

Copy link
Contributor

@agoscinski agoscinski left a comment

Choose a reason for hiding this comment

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

My main question about the position of set_progress_bar_tqdm is: Should a user coming from the Python API also see the progress bar? If yes, then this needs to be in maintain. Maybe one can stick for this PR with the consistency, if one does not have an answer to this question.

src/aiida/cmdline/commands/cmd_storage.py Show resolved Hide resolved
@khsrali
Copy link
Contributor Author

khsrali commented Sep 17, 2024

My main question about the position of set_progress_bar_tqdm is: Should a user coming from the Python API also see the progress bar? If yes, then this needs to be in maintain. Maybe one can stick for this PR with the consistency, if one does not have an answer to this question.

Agreed.

Copy link

codecov bot commented Sep 19, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.

Project coverage is 77.84%. Comparing base (ef60b66) to head (2946b78).
Report is 112 commits behind head on main.

Files with missing lines Patch % Lines
src/aiida/cmdline/commands/cmd_storage.py 83.34% 1 Missing ⚠️
src/aiida/orm/utils/serialize.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6562      +/-   ##
==========================================
+ Coverage   77.51%   77.84%   +0.34%     
==========================================
  Files         560      566       +6     
  Lines       41444    41994     +550     
==========================================
+ Hits        32120    32685     +565     
+ Misses       9324     9309      -15     

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

@khsrali
Copy link
Contributor Author

khsrali commented Sep 19, 2024

yeah, yeah, CI fails because disck-objectstore v1.1.1 cannot be found in conda-forge
Opened a PR on that conda-forge/disk-objectstore-feedstock#5

@khsrali
Copy link
Contributor Author

khsrali commented Sep 20, 2024

@agoscinski I see mypy is complaining:

src/aiida/orm/utils/serialize.py:51: error: Unused "type: ignore[assignment]" comment  [unused-ignore]
Found 1 error in 1 file (checked 784 source files)

But I've not changed this file at all.. ? 🤔

This is crazy, I run it local by removing assignment, and then it complains 🙃

src/aiida/orm/utils/serialize.py:51: error: Incompatible types in assignment (expression has type "Union[str, int, float, None]", variable has type "str")  [assignment]
src/aiida/orm/utils/serialize.py:51: note: Error code "assignment" not covered by "type: ignore" comment
Found 1 error in 1 file (checked 1 source file)

Update all requirements files........................(no files to check)Skipped
Validate environment.yml.............................(no files to check)Skipped
Automatically generating verdi docs..................(no files to check)Skipped

@GeigerJ2
Copy link
Contributor

src/aiida/orm/utils/serialize.py:51: error: Unused "type: ignore[assignment]" comment  [unused-ignore]
Found 1 error in 1 file (checked 784 source files)

I was actually having exactly the same error on my other PR #6565...

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.

4 participants