Refactor archive progress bar #4504

chrisjsewell · 2020-10-23T05:38:35Z

As proposed in #4053 (comment), this PR refactors the progress bar implementation in for the archive export to remove the singleton approach and make all calls to the progress bar a context manager.
I also improved some progress bars to give better feedback (in particular the entity field collection).

I've tested this implementation (see below) and find no issues.

As an added bonus, I have also added a verbose option to the CLI (-v), such that this will set the progress bars to be left on screen (i.e. leave=True)

chrisjsewell · 2020-10-23T05:44:24Z

codecov · 2020-10-23T06:04:06Z

Codecov Report

Merging #4504 into develop will decrease coverage by 0.01%.
The diff coverage is 94.84%.

@@             Coverage Diff             @@
##           develop    #4504      +/-   ##
===========================================
- Coverage    79.36%   79.36%   -0.00%     
===========================================
  Files          475      476       +1     
  Lines        34907    34950      +43     
===========================================
+ Hits         27701    27733      +32     
- Misses        7206     7217      +11

Flag	Coverage Δ
#django	`73.22% <93.90%> (+0.01%)`	⬆️
#sqlalchemy	`72.45% <93.90%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida/tools/importexport/common/config.py	`100.00% <ø> (ø)`
aiida/common/progress_reporter.py	`71.88% <71.88%> (ø)`
aiida/tools/importexport/dbexport/__init__.py	`97.80% <98.73%> (-0.26%)`	⬇️
aiida/cmdline/commands/cmd_export.py	`90.91% <100.00%> (+0.47%)`	⬆️
aiida/common/log.py	`95.59% <100.00%> (+0.07%)`	⬆️
aiida/tools/importexport/common/progress_bar.py	`83.34% <100.00%> (ø)`
.../importexport/dbimport/backends/django/__init__.py	`91.55% <100.00%> (+0.03%)`	⬆️
...ls/importexport/dbimport/backends/sqla/__init__.py	`92.52% <100.00%> (+0.03%)`	⬆️
aiida/transports/plugins/local.py	`82.57% <0.00%> (-0.25%)`	⬇️

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 1be12e1...694fb47. Read the comment docs.

ltalirz · 2020-10-23T08:28:22Z

Hi @chrisjsewell, before going through the code in detail, here is a question:

There is an open PR #3896 to homogenize verbosity handling across the whole verdi cli.
As is common with loggers, the approach taken there is to configure a VERDI_LOGGER singleton instance, which then has the advantage that one can simply VERDI_LOGGER.debug(...) or VERDI_LOGGER.warn(...).

I am wondering whether a similar approach would not work for the progress bar as well.
Is it really necessary to pass the instance around to every little sub function?
And by having the progress bar as a required argument we force anyone who would like to use these functions to set up a fake progress bar just to call it.

chrisjsewell · 2020-10-23T10:27:30Z

I am wondering whether a similar approach would not work for the progress bar as well.
Is it really necessary to pass the instance around to every little sub function?
And by having the progress bar as a required argument we force anyone who would like to use these functions to set up a fake progress bar just to call it.

Eurh, I think exactly the opposite; I think singleton's (and using mutable global variables in general) are awful and should only be used when absolutely necessary. I would point you, for example, towards https://stackoverflow.com/a/138012/5033292, https://stackoverflow.com/a/2663030/5033292 etc, for the multiple issues with the singleton design pattern

Basically I would advocate dependency injection over the singleton design pattern

chrisjsewell · 2020-10-23T10:35:54Z

if anything, the private functions in this class + the export function should be turned into a class, with a shared (local) state.
I would be happy to make this change

ltalirz · 2020-10-23T12:49:42Z

I think singleton's (and using mutable global variables in general) are awful and should only be used when absolutely necessary.

I think, as with many questions, there is a balance to be found.
Of course I'm aware of the drawbacks of global variables - at the same time, passing every variable around every time can hurt code readability.

Using loggers is a standard approach suggested in the official python tutorial https://docs.python.org/3/howto/logging.html#configuring-logging
Would you suggest that we start passing loggers around explicitly in aiida-core?
[in the context of functional programming, one eventually ends up in the question of whether to pass around "the screen" ;-) ]

In any case, I agree that the progress bar (which is around only temporarily and specific to this section of hte code) differs from loggers (which are assumed to be there all the time), so perhaps we will need to pass it around in the end.

if anything, the private functions in this class + the export function should be turned into a class, with a shared (local) state.
I would be happy to make this change

Yes, a more localized way of sharing state would make sense.
Whatever the solution, it would be helpful to have an obvious way of using functions that can interact with the progress bar also in scenarios where one doesn't have a progress bar at hand (unless they are really so specific that we cannot imagine anyone ever using them in a different context).

chrisjsewell · 2020-10-23T13:10:42Z

Would you suggest that we start passing loggers around explicitly in aiida-core?

Well loggers are one of them rare use cases. Although still it can be a real pain to ensure the behaviour of logging, for example sphinx sets up a global logger which makes debugging failed tests a real annoyance (docutils/sphinx also have some other singletons which are horrible to override, and one of the reasons for my hatred of them lol).

chrisjsewell · 2020-10-23T13:47:35Z

it would be helpful to have an obvious way of using functions that can interact with the progress bar also in scenarios where one doesn't have a progress bar at hand

Just to note, none of the public functions require that a progress bar be specifically passed

aiida/tools/importexport/dbexport/__init__.py

ltalirz

thanks @chrisjsewell

only minor comments

aiida/cmdline/commands/cmd_export.py

aiida/tools/importexport/dbexport/__init__.py

ltalirz · 2020-10-23T14:21:21Z

aiida/tools/importexport/dbexport/__init__.py

-        )
-        res = set(builder.all(flat=True))
-        given_log_entry_ids.update(res)
+    total = 1 + ((1 if include_logs else 0) + (1 if include_logs else 0) if node_ids_to_be_exported else 0)


that should probably be

Suggested change

total = 1 + ((1 if include_logs else 0) + (1 if include_logs else 0) if node_ids_to_be_exported else 0)

total = 1 + ((1 if include_logs else 0) + (1 if include_comments else 0) if node_ids_to_be_exported else 0)

by the way, if there are no node ids to be exported, what is the 1 for?

oops, yeh I was having an issue without the 1, but I will re-check

chrisjsewell · 2020-10-24T00:33:12Z

@ltalirz inspired by the discussion in #3896, I have re-re-factored in a54c319!

The progress bar is once again a singleton, but now it is fully generalised to just the concept of a progress reporter. As such it is not specific to importexport and so I have added its code as aiida/common/progress_reporter.py.

Control of setting the progress reporter is now moved to the CLI (perhaps it could somehow be integrated into #3896?), the exporter functions just retrieve it, with the default being a "null" reporter.

For now aiida/tools/importexport/common/progress_bar.py remains, but in the import refactor, this should be removed

aiida/cmdline/commands/cmd_export.py

aiida/tools/importexport/dbexport/__init__.py

chrisjsewell · 2020-10-24T00:39:54Z

Note there is some code to set/reset the logging level in the export function, which could maybe be removed in #3896

chrisjsewell · 2020-10-24T02:03:57Z

In 19da2ad I have also moved control of the logger level (and formatting in 81f2118) to the CLI, so it should now be very easy to add in the #3896 code later

chrisjsewell · 2020-10-24T02:06:19Z

$ verdi export create --input-calc-forward -N 1 -G 1 -f -v DEBUG -- tmp/mount_folder/out2.aiida 
|
EXPORT
--------------  ---------------------------
Archive         tmp/mount_folder/out2.aiida
Format          Zip (compressed)
Export version  0.9

Inclusion rules
-----------------  ----
Include Comments   True
Include Logs       True

Traversal rules
---------------------------------  -----
Follow links input calc forwards   True
Follow links input calc backwards  True
Follow links create forwards       True
Follow links create backwards      True
Follow links return forwards       True
Follow links return backwards      False
Follow links input work forwards   False
Follow links input work backwards  True
Follow links call calc forwards    True
Follow links call calc backwards   True
Follow links call work forwards    True
Follow links call work backwards   True

STARTING EXPORT...
Retrieving Nodes from Groups ...         100.0%|█████████████████████████████████████████████████████| 2/2
Traversing provenance via links ...      100.0%|█████████████████████████████████████████████████████| 1/1
Initializing export of all entities      100.0%|█████████████████████████████████████████████████████| 3/3
Preparing entities - Groups              100.0%|█████████████████████████████████████████████████████| 2/2
GATHERING DATABASE ENTRIES...
Exporting Group fields                   100.0%|█████████████████████████████████████████████| 40066/40066
Exporting a total of 40067 database entries, of which 40065 are Nodes.
GATHERING GROUP ELEMENTS...
Exporting Groups ...                     100.0%|█████████████████████████████████████████████| 40065/40065
GATHERING NODE ATTRIBUTES AND EXTRAS...
Exporting Attributes and Extras          100.0%|█████████████████████████████████████████████| 40065/40065
Data extracted in     34 s.
ADDING DATA TO EXPORT ARCHIVE...
ADDING REPOSITORY FILES TO EXPORT ARCHIVE...
Exporting repository - UUID=f191b6cb     100.0%|█████████████████████████████████████████████| 40065/40065
Data written in 2.4e+02 s.
Success: wrote the export archive file to tmp/mount_folder/out2.aiida

ltalirz

Hi @chrisjsewell , thanks for putting more thought into this an coming up with this solution.

I have some questions concerning the interface before finishing the review; will get back to the laptop later tonight

aiida/common/log.py

aiida/common/progress_reporter.py

aiida/common/log.py

ltalirz

Thanks @chrisjsewell ; mostly requests for documentation + one simplification

ltalirz · 2020-10-24T21:47:24Z

aiida/common/progress_reporter.py

+
+    Example Usage::
+
+        with progress_reporter(total=10, desc="A process:") as progress:


Is this supposed to be progress_reporter_base?

aiida/common/progress_reporter.py

aiida/tools/importexport/dbexport/__init__.py

aiida/common/progress_reporter.py

chrisjsewell · 2020-10-25T00:04:16Z

@ltalirz in 1ab807a I have re-written the progress reporter contextmanager as a single class.
It works exactly the same as the decorated function, but I hope is a bit more easy to understand (it also works properly with auto-completion).

I have also added the set_progress_bar_tqdm convenience function

ltalirz

Thanks @chrisjsewell , at least to me this version of the progress reporter is much more obvious to interact with.

Let's merge this :-)
Edit: After the tests pass ;-)

aiida/tools/importexport/common/progress_bar.py

aiida/tools/importexport/dbexport/__init__.py

Refactor export progress bar

01ad67a

fix doc warnings

a3356a0

chrisjsewell requested a review from ltalirz October 23, 2020 05:47

chrisjsewell mentioned this pull request Oct 23, 2020

Callback concept for the progress bar #4053

Closed

chrisjsewell added 2 commits October 23, 2020 14:33

move prograss_context injection to the writer init

3ec26d3

Allow the tar archive to be created independently of an aiida repo

9fca8ce

Merge branch 'develop' into export/progress-bar

4d3d552

chrisjsewell commented Oct 23, 2020

View reviewed changes

aiida/tools/importexport/dbexport/__init__.py Outdated Show resolved Hide resolved

ltalirz suggested changes Oct 23, 2020

View reviewed changes

chrisjsewell added 2 commits October 24, 2020 02:19

re-re-factor progress bar!

a54c319

update help text for verbose option

9e552b5

chrisjsewell requested a review from ltalirz October 24, 2020 00:33

chrisjsewell commented Oct 24, 2020

View reviewed changes

aiida/cmdline/commands/cmd_export.py Outdated Show resolved Hide resolved

chrisjsewell commented Oct 24, 2020

View reviewed changes

aiida/tools/importexport/dbexport/__init__.py Show resolved Hide resolved

chrisjsewell added 2 commits October 24, 2020 02:45

small update

b853e8c

Move control of logging verbosity to CLI

19da2ad

chrisjsewell added 2 commits October 24, 2020 04:43

Move log formatting control to CLI

81f2118

Move partial to set_progress_reporter

16744d3

ltalirz reviewed Oct 24, 2020

View reviewed changes

aiida/common/log.py Outdated Show resolved Hide resolved

aiida/common/progress_reporter.py Show resolved Hide resolved

chrisjsewell commented Oct 24, 2020

View reviewed changes

aiida/common/log.py Outdated Show resolved Hide resolved

chrisjsewell added 2 commits October 24, 2020 21:56

Apply suggestions from code review

286e865

fix description

78e9e79

ltalirz suggested changes Oct 24, 2020

View reviewed changes

chrisjsewell mentioned this pull request Oct 24, 2020

Refactor Import Archive #4510

Merged

chrisjsewell commented Oct 24, 2020

View reviewed changes

aiida/common/progress_reporter.py Outdated Show resolved Hide resolved

rewrite progress reporter as class

1ab807a

chrisjsewell requested a review from ltalirz October 25, 2020 00:01

minor fix

8d0afa1

ltalirz previously approved these changes Oct 25, 2020

View reviewed changes

aiida/tools/importexport/common/progress_bar.py Show resolved Hide resolved

chrisjsewell commented Oct 25, 2020

View reviewed changes

aiida/tools/importexport/dbexport/__init__.py Outdated Show resolved Hide resolved

fix accidental change (whilst testing)

694fb47

chrisjsewell dismissed ltalirz’s stale review via 694fb47 October 25, 2020 10:24

ltalirz self-requested a review October 25, 2020 10:33

ltalirz approved these changes Oct 25, 2020

View reviewed changes

chrisjsewell changed the title ~~Refactor export progress bar~~ Refactor archive progress bar Oct 25, 2020

chrisjsewell merged commit 33c9f41 into aiidateam:develop Oct 25, 2020

chrisjsewell deleted the export/progress-bar branch October 25, 2020 11:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor archive progress bar #4504

Refactor archive progress bar #4504

chrisjsewell commented Oct 23, 2020

chrisjsewell commented Oct 23, 2020

codecov bot commented Oct 23, 2020 •

edited

Loading

ltalirz commented Oct 23, 2020 •

edited

Loading

chrisjsewell commented Oct 23, 2020 •

edited

Loading

chrisjsewell commented Oct 23, 2020 •

edited

Loading

ltalirz commented Oct 23, 2020 •

edited

Loading

chrisjsewell commented Oct 23, 2020

chrisjsewell commented Oct 23, 2020

ltalirz left a comment

ltalirz Oct 23, 2020

ltalirz Oct 23, 2020

chrisjsewell Oct 23, 2020

chrisjsewell commented Oct 24, 2020 •

edited

Loading

chrisjsewell commented Oct 24, 2020

chrisjsewell commented Oct 24, 2020 •

edited

Loading

chrisjsewell commented Oct 24, 2020 •

edited

Loading

ltalirz left a comment

ltalirz left a comment

ltalirz Oct 24, 2020

chrisjsewell commented Oct 25, 2020 •

edited

Loading

ltalirz left a comment •

edited

Loading

	total = 1 + ((1 if include_logs else 0) + (1 if include_logs else 0) if node_ids_to_be_exported else 0)
	total = 1 + ((1 if include_logs else 0) + (1 if include_comments else 0) if node_ids_to_be_exported else 0)


		Example Usage::

		with progress_reporter(total=10, desc="A process:") as progress:

Refactor archive progress bar #4504

Refactor archive progress bar #4504

Conversation

chrisjsewell commented Oct 23, 2020

chrisjsewell commented Oct 23, 2020

codecov bot commented Oct 23, 2020 • edited Loading

Codecov Report

ltalirz commented Oct 23, 2020 • edited Loading

chrisjsewell commented Oct 23, 2020 • edited Loading

chrisjsewell commented Oct 23, 2020 • edited Loading

ltalirz commented Oct 23, 2020 • edited Loading

chrisjsewell commented Oct 23, 2020

chrisjsewell commented Oct 23, 2020

ltalirz left a comment

Choose a reason for hiding this comment

ltalirz Oct 23, 2020

Choose a reason for hiding this comment

ltalirz Oct 23, 2020

Choose a reason for hiding this comment

chrisjsewell Oct 23, 2020

Choose a reason for hiding this comment

chrisjsewell commented Oct 24, 2020 • edited Loading

chrisjsewell commented Oct 24, 2020

chrisjsewell commented Oct 24, 2020 • edited Loading

chrisjsewell commented Oct 24, 2020 • edited Loading

ltalirz left a comment

Choose a reason for hiding this comment

ltalirz left a comment

Choose a reason for hiding this comment

ltalirz Oct 24, 2020

Choose a reason for hiding this comment

chrisjsewell commented Oct 25, 2020 • edited Loading

ltalirz left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Oct 23, 2020 •

edited

Loading

ltalirz commented Oct 23, 2020 •

edited

Loading

chrisjsewell commented Oct 23, 2020 •

edited

Loading

chrisjsewell commented Oct 23, 2020 •

edited

Loading

ltalirz commented Oct 23, 2020 •

edited

Loading

chrisjsewell commented Oct 24, 2020 •

edited

Loading

chrisjsewell commented Oct 24, 2020 •

edited

Loading

chrisjsewell commented Oct 24, 2020 •

edited

Loading

chrisjsewell commented Oct 25, 2020 •

edited

Loading

ltalirz left a comment •

edited

Loading