search

Cadair / parfive

An asyncio based parallel file downloader for Python 3.8+
https://parfive.readthedocs.io/
MIT License
50 stars 24 forks source link

customizable callback when download completed #127

Closed dreamflasher closed 5 months ago

dreamflasher commented 1 year ago

Closes https://github.com/Cadair/parfive/issues/123

Cadair commented 1 year ago

On a higher level, I am wondering what the advantage to this pattern vs inspecting the returned Results object is. This adds another reasonable size cunk to our user facing API and I am not clear what functionality it's adding.

dreamflasher commented 1 year ago

On a higher level, I am wondering what the advantage to this pattern vs inspecting the returned Results object is. This adds another reasonable size cunk to our user facing API and I am not clear what functionality it's adding.

Thank you for asking! Inspecting the Results object is only possible if it runs until the expection – so all downloads need to be fully processed. If the process is killed meanwhile, that won't happen. But it's your call if you find it useful enough. If you do, I'll make the changes you requested – I agree with them :)

Cadair commented 1 year ago

If the process is killed meanwhile, that won't happen.

That's a fair point, and I guess in an async use case you might end up wanting to do some kind of work after each file is downloaded. I would be happy to add this. I would want to understand and test what happens to the callback if the download is cancelled part way through. Do we always execute the callback or do we cancel that as well?

dreamflasher commented 1 year ago

Do we always execute the callback or do we cancel that as well?

Well, we should think about how we want to design it. Currently, I implemented it that the callback is always called – and in case the download was cancelled we call the callback with the Error. Then you have the flexibility when you implement the callback, to just ignore the error cases, or handle them (do cleanup etc.).

codecov[bot] commented 1 year ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 90.54%. Comparing base (3b049c5) to head (ea1ca05). Report is 4 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #127 +/- ## ========================================== + Coverage 90.23% 90.54% +0.30% ========================================== Files 5 5 Lines 635 645 +10 ========================================== + Hits 573 584 +11 + Misses 62 61 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Cadair commented 1 year ago

I think we probably want the callbacks to execute even as we are cleaning up the call stack. No idea how we go about testing that though.

dreamflasher commented 1 year ago

@Cadair I added a test for the callback, I use Python 3.11 and locally it works fine – also in your test suite. I don't understand the error that's thrown for py38-conda (why does it work fine for py38?). Would you be so kind as to have a look?

I also think the callback should be called when cleaning up the call stack… but yes, I also don't know how to test this.

Cadair commented 1 year ago

I have a couple of busy weeks of work coming up but I will try and get back to this soon.

dreamflasher commented 10 months ago

@Cadair there doesn't seem to exist a simple way to raise an exception after calling a function?

Cadair commented 10 months ago

I am not sure I follow what you are asking, where do you want to raise the exception?

dreamflasher commented 10 months ago

I am not sure I follow what you are asking, where do you want to raise the exception?

Ah sorry, I was jumping ahead: The only check that's not successful is the code coverage and it complains that the exception handling is not tested. I had a look if there's an easy way to test exception handling, but it doesn't seem to be possible. The problem is: The function needs to be running for some time, and then it's getting cancelled. Then one could test the exception handling.

Cadair commented 10 months ago

This test: https://github.com/Cadair/parfive/blob/main/parfive/tests/test_downloader.py#L324 and specifically the fixture it uses: https://github.com/Cadair/parfive/blob/main/parfive/conftest.py#L9 is rigged up to throw an error every second download. You should be able to reuse that to test that your callback gets passed an exception if a download fails.

dreamflasher commented 10 months ago

@Cadair Thank you! Added the test. Do you have any idea why the build is not working?

Cadair commented 10 months ago

The publish build is failing because of an unrelated Python 3.12 issue.

dreamflasher commented 10 months ago

I guess then this is ready to be merged? :)

dreamflasher commented 10 months ago

nonlocal didn't work, so I went with temp files

dreamflasher commented 10 months ago

@Cadair would you be down for merging it?

Cadair commented 5 months ago

Thanks a lot @dreamflasher :smile: