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

[khsrali]

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]

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]

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]

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

[khsrali]

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

ok, pls check now.

[khsrali]

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.

[agoscinski]

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.

[khsrali]

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.

[codecov]

Codecov Report

Attention: Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Project coverage is 77.85%. Comparing base (ef60b66) to head (0f5411c).
Report is 161 commits behind head on main.

Files with missing lines	Patch %	Lines
src/aiida/cmdline/commands/cmd_storage.py	83.34%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6562      +/-   ##
==========================================
+ Coverage   77.51%   77.85%   +0.35%     
==========================================
  Files         560      566       +6     
  Lines       41444    42049     +605     
==========================================
+ Hits        32120    32734     +614     
+ Misses       9324     9315       -9     

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

[khsrali]

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]

@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]

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...

[unkcpz]

I don't think you need to change this. We don't want to pin the patch version. Same for the environment.yml above.

[khsrali]

Thanks @unkcpz for the review

I understand that disk-objectstore~=1.1 would automatically install the latest patch release.
However, I think it's better to clearly specify the minimum compatible version, as 1.1.0 is not compatible.

This is important because if a user already has disk-objectstore v1.1.0 installed and loses internet access during the installation of aiida-core, the installation will proceed with disk-objectstore v1.1.0 without requiring it to update to 1.1.1.
In that case, we have a wrong installation, with no warning or whatsoever that leads to a bunch of errors emerging during the runtime.

[unkcpz]

as 1.1.0 is not compatible.

What you mean by "not compatible"? If it is a release that have problem then maybe we need to bring it down from pypi. If 1.1.1 is just adding new feature then having v1.1.0 should be fine I guess? Maybe I miss something?

[unkcpz]

Ah, I see. The implementation here is rely on the change with the release 1.1.1.
Then if we follow the semver ("MINOR version when you add functionality in a backward compatible manner"), that one should be released as a minor version bump.
I don't know how much we want to follow the semantic versioning. The problem with pinning to patch version is if we bring new things in the future to disk-objectstore and release as 1.2 or higher, it is always needed to bump the version in aiida-core and make release to bring it to user which may not ideal.

[unkcpz]

I just did a read on https://github.com/aiidateam/AEP/blob/master/002_dependency_management/readme.md again.
If we gonna to follow it, then we probably need to release v1.2 and using ~=1.2 in aiida-core.

[khsrali]

Ok, I understand that should have been released as 1.2.
But now, given the situation I don't really think we need to re-release disk-objectstore just to follow semantic versioning thing.

Time and effort it requires, does payoff for the fruit.

[agoscinski]

I think @unkcpz has a point. We cannot fix this anymore backwards, but lets at least to quickly a release.
aiidateam/disk-objectstore#177

Since not every package is our control I would just do a comment an the dependencies, but since we can make quickly a release, I would fix it, because this has to happen anyway.

[khsrali]

Then please also fix these as well:

• plumpy~=0.22.3
• kiwipy[rmq]~=0.8.4
• archive-path~=0.4.2
• click-spinner~=0.1.8
• upf_to_json~=0.9.2

you may re-release kiwipy, plumpy, and archive-path

[agoscinski]

I can understand the frustration, but that is not in the scope of this PR to solve other peoples mistakes, but in this case we did a wrong release.

[unkcpz]

@khsrali just one minor request, all good then. Thanks!

[agoscinski]

Suggested change

	disk-objectstore==1.1.1
	disk-objectstore~=1.2

[unkcpz]

This file (and other requirements below) are initially generated by pip-compile and the version is sepcific to patch version, so it needs to be 1.2.0.

[unkcpz]

Just a thought in my mind, I think pip-compile --extra=atomic_tools --extra=docs --extra=notebook --extra=rest --extra=tests --no-annotate --output-file=requirements/requirements-py-3.9.txt pyproject.toml command to generate this file needs to be run by CI when there are dependencies changes in project.toml. Anyway not related to this PR.

[agoscinski]

Ah I see these are lock files. Thanks will undo this

[agoscinski]

Yes or a pre-commit hook that checks diff (which is then basically run by the CI^^)

[unkcpz]

pre-commit ... Seems you didn't success on pushing to move away from it 🤔

[agoscinski]

promise aiida-core 3.0 no pre-commit

[unkcpz]

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

I missed the comment here, sorry. I think @agoscinski here hit the point, the context manager should appear not in the backend so there is more control on whether to show the progress or not.

Note the context manager should remain in the backend, as that's the right use of it.

@khsrali this I may not agree, if you look at with open(), it is usually used directly. Sorry, I mess up the concept of context management you mentioned. Here we are talking about logging level.

[unkcpz]

Should all good now I guess? Thanks @khsrali @agoscinski!