[#574] Allow for tqdm progress bars to be used

[qubixes]

Works for both parallel as well as single threaded gets and puts. Some points still to be discussed:

• [ ] Tests: I could write tests with a tqdm progress bar, but that would include an extra dependency for tests.
• [ ] There is already some mechanism for progress bars in the PRC, so for now these two methods are living next to each other. I don't know if that is a problem.
• [ ] I changed pbar -> progress_bar, which seems more clear for user facing methods.

[d-w-moore]

@qubixes - good point about the testing and extra dependencies. I think it's possible we could do a mock of the tqdm progress bar, however, since its interface (the update(nbytes) method) is so simple. In fact, I'm wondering if maybe the progress_bar parameter itself could be altered so we're taking a callable instead of a progress-bar object per se. That could make the PRC more generic & possibly compatible with other progress bar libraries. I may make a stab at it.

[d-w-moore]

• [] There is already some mechanism for progress bars in the PRC, so for now these two methods are living next to each other. I don't know if that is a problem.

I am now thinking the existing progress-bar support and mechanism could be deprecated, and soon after that removed, since it was originally written for demo and is not used by anyone, so far as I know.

The transfer-done callback would remain, since it can notify the client that a data_object, after being written, is also closed and therefore ready for other agents to read/write freely.

[trel]

using a tried-tested external library for this seems agreeable. can we just ... swap to using tqdm? doesn't seem a risk.

https://github.com/tqdm/tqdm

[d-w-moore]

using a tried-tested external library for this seems agreeable. can we just ... swap to using tqdm? doesn't seem a risk.

https://github.com/tqdm/tqdm

Yes, maybe... On the other hand, it is possible a client might want several update functions to be called during a transfer, including possibly custom ones. So an argument of say update_functions that takes a list or tuple of callables isn't too outlandish.

Just a thought.

So, we can go ahead with progress_bar arg (leave things as they are) and just have it understood it takes a tqdm object, or we can try and make it more generic, entailing pretty minimal changes to this PR. Opinions?

[trel]

let's try your generic experiment and see how it feels.

[korydraughn]

Let's be generic where possible.

[qubixes]

using a tried-tested external library for this seems agreeable. can we just ... swap to using tqdm? doesn't seem a risk. https://github.com/tqdm/tqdm

Yes, maybe... On the other hand, it is possible a client might want several update functions to be called during a transfer, including possibly custom ones. So an argument of say update_functions that takes a list or tuple of callables isn't too outlandish.

Just a thought.

So, we can go ahead with progress_bar arg (leave things as they are) and just have it understood it takes a tqdm object, or we can try and make it more generic, entailing pretty minimal changes to this PR. Opinions?

I think that from a technical perspective either solution could work. For multiple update functions one could use either inheritance (all untested):

class MyUpdateClass(tqdm):
    def update(self, *args, **kwargs):
          some_update_func()
          some_other_update_func()
          super().update(*args, **kwargs)

Or encapsulation (also untested):

class MyUpdateClass():
    def __init__(self, pbar):
          self.pbar = pbar
    def update(self, *args, **kwargs):
          some_update_func()
          some_other_update_func()
          self.pbar.update(*args, **kwargs)

From the perspective of iBridges, the current solution is of course easier to implement, but another interface would not be so hard to implement either. So I think it's more how you feel the functionality will/should be used by the user and which design fits that philosophy best. One thing that might be worth considering is that the update functions that the user provide should be thread safe, which might place some burden on the user to know what they are doing.

[korydraughn]

Correct me if I'm wrong, but it seems all the PRC needs to do is accept a single object that's invocable as a function. That grants the user great flexibility. There's also no need to accept a list/tuple of callables because the passed callable can do whatever it wants. The only requirements would be:

• The callable must be thread-safe
• The callable must accept the number of bytes to add to the progress
  • Or some other value which enables the progress bar to be updated correctly

For example:

def progress_tracker(delta):
    # These functions could touch completely different objects
    # within an application.
    some_func1(delta)
    some_func2(delta)
    some_funcN(delta)

Other things to consider are:

• Should the callable always be responsible for handling synchronization?
  • Perhaps the PRC can handle this for the user?
• Are there cases where each internal function must use their own synchronization mechanism?

[d-w-moore]

@qubixes @korydraughn - good points all. I'm already on a track toward fulfilling a lot of these concerns, I believe, in this branch: https://github.com/d-w-moore/python-irodsclient/tree/add-tqdm-pbar .

Central ideas:

• allow synchronization / thread safety
• accept a chunk size in bytes and update objects appropriately
• mainly allow for callables in the list of updatable objects submitted to put / get, but allow registering other types (like progressbar.ProgressBar or tqdm.tqdm) . Callables, when submitted, should return a Pythonically bound instance (i.e. having a fixed, implicit self value) of the update method that updates a particular UI object.

Trying to get this done today. It's in testing phase currently. I may add to the README too, and make some of the less obvious points of its usage clearer....

[korydraughn]

Excellent. Yes, please update the README so users can be confident in their usage of the feature.

[d-w-moore]

@qubixes - https://github.com/d-w-moore/python-irodsclient/tree/add-tqdm-pbar now contains a reasonably generic implementation (your commit, plus some of mine that built on your work) which can be used in way closely mimicking the progress_bar parameter in your development branch. In other words, if you call session.data_objects.put (or get) with the updatables = [ tqdm_instance.update ], it should behave the same as with the use of progress_bar = tqdm_instance in your implementation. Please test it out and let me know if you run into any hiccups. It should behave the same as it did in your version, just with that renaming of the call parameter .

[qubixes]

@d-w-moore I have checked both up- and downloads in tqdm/iBridges, 25MB and 200MB. No hiccups on my end. Thanks for continuing the implementation! Do you need anything more from me at this point?

[d-w-moore]

@d-w-moore I have checked both up- and downloads in tqdm/iBridges, 25MB and 200MB. No hiccups on my end. Thanks for continuing the implementation! Do you need anything more from me at this point?

That's good to hear!

Just your preference on how to handle the pull request from this point forward, I suppose. Any objection to my replacing your PR with one of my own? Your commit will be preserved to give you credit, of course : )

[qubixes]

@d-w-moore I have checked both up- and downloads in tqdm/iBridges, 25MB and 200MB. No hiccups on my end. Thanks for continuing the implementation! Do you need anything more from me at this point?

That's good to hear!

Just your preference on how to handle the pull request from this point forward, I suppose. Any objection to my replacing your PR with one of my own? Your commit will be preserved to give you credit, of course : )

No, go ahead!