search

irods / python-irodsclient

A Python API for iRODS
Other
62 stars 72 forks source link

Allow tqdm progress bars to be used with python-irodsclient #574

Open qubixes opened 1 week ago

qubixes commented 1 week ago

Currently within iBridges we don't have a way to update our progress bar during large transfers. I found that there is a progress bar/queue in the parallel transfer module, but using that in combination with tqdm or any external progress bar seems difficult, since it is buried quite deeply. Apart from the CLI/API, we would also like to have a continuously updating progress bar for the GUI (which might not use tqdm).

I have created a branch in my fork that showcases a possible solution: https://github.com/qubixes/python-irodsclient/tree/add-tqdm-pbar. In combination with an iBridges PR (https://github.com/UtrechtUniversity/iBridges/pull/209) I managed to get reliable updates during transfers (up/download). The solution adds an extra parameter pbar to the arguments of put/get in the data_object_manager.

Let me know if/when you want me to create a PR from my branch.

Linked to https://github.com/UtrechtUniversity/iBridges/issues/202

trel commented 1 week ago

current diff is here

https://github.com/irods/python-irodsclient/compare/main...qubixes:python-irodsclient:add-tqdm-pbar

d-w-moore commented 1 week ago

This seems as good an addition as the existing progress bar infrastructure. I think we could absorb these changes as-is for the current release.

In the long run, it may be good to absorb extra options (those not directly relevant to the parallel transfer itself) into a keyword argument parameter (**_extra_options as an example) into each thread via the submit method of the executor, then passed down implicitly to each subcall likewise in a **-style argument ... which might let us more easily absorb changes for other third-party libs. opinions @trel ?

trel commented 1 week ago

This seems as good an addition as the existing progress bar infrastructure. I think we could absorb these changes as-is for the current release.

That sounds good.

Do we need a test for this new progress bar?

d-w-moore commented 1 week ago

This seems as good an addition as the existing progress bar infrastructure. I think we could absorb these changes as-is for the current release.

That sounds good.

Do we need a test for this new progress bar?

I would think so, either for the progress bar or for whatever capability we end up with to support progress bars generically. @qubixes , do you have any tests that would or could be included in the PR?

qubixes commented 1 week ago

This seems as good an addition as the existing progress bar infrastructure. I think we could absorb these changes as-is for the current release.

That sounds good. Do we need a test for this new progress bar?

I would think so, either for the progress bar or for whatever capability we end up with to support progress bars generically. @qubixes , do you have any tests that would or could be included in the PR?

Thanks for the interest in including this in the PRC! I could write some tests for the progress bar, however, those would create a tqdm requirement for testing and I'm not sure what the policy on that is. I'll create the draft PR and we can discuss the details of the PR there.