[ENH] Handle Additional Kwargs in AiiDA Progress Reporter #6757
Conversation
|
@khsrali could you give it a look? The callback seems a bit anti-pattern, I may prefer to go with dependency injection for the original reporter implementation in disk-objectstore. But since it is already there, I may don't have time to take a look at further change. Much appreciate if you can take care of this. |
|
So is this implementation fine for now? @khsrali |
There was a problem hiding this comment.
Thanks @jgyasu
You can start writing tests, maybe also try with manual tests.
As this is more of a visual and graphical thing.
The idea, was to allow some keywords pass to the progress reporter. The progress reported may or not be tqdm. In case it's tqdim it has to check if those keywords are legit and if they are not in conflict with this:
TQDM_BAR_FORMAT = '{desc:40.40}{percentage:6.1f}%|{bar}| {n_fmt}/{total_fmt}'
If they are in conflict, either raise, or think how to solve it.
| merged_kwargs.update(init_kwargs) | ||
| return tqdm(*args, **merged_kwargs) | ||
|
|
||
| set_progress_reporter(tqdm_with_kwargs, bar_format=bar_format, leave=leave) |
There was a problem hiding this comment.
Not sure, if this works.. As you already initialize it
There was a problem hiding this comment.
You may pass the class when it's not initialized, yet.
| """Return the additional keyword arguments.""" | ||
| return self._kwargs | ||
|
|
||
| def update_kwargs(self, **kwargs: Any) -> None: |
There was a problem hiding this comment.
The given keywords should not conflict with TQDM_BAR_FORMAT
| progress_reporter.reset(value['total']) | ||
| progress_reporter.set_description_str(value['description']) | ||
| total = value.pop('total', None) | ||
| description = value.pop('description', None) |
There was a problem hiding this comment.
Why do you need to change this? there will always be a value in the way the _callback supposed to be utilized
| progress_reporter.set_description_str(description) | ||
|
|
||
| if value: | ||
| progress_reporter.update_kwargs(**value) |
There was a problem hiding this comment.
don't really need to update this.
Also this keywords are not exactly one-to-one to tqdm keys
Fixes #6564