Add --start-idx=<n>, --end-idx=<m> option to enable ranged downloads #605
Conversation
Russtopia
force-pushed
the
master
branch
2 times, most recently
from
994385e
to
1658b33
Compare
Russtopia
changed the title
Russtopia
force-pushed
the
master
branch
2 times, most recently
from
69627ae
to
40450f9
Compare
There was a problem hiding this comment.
Thank you for the patch.
You should really run the tests locally against your changes. See https://github.com/jjjake/internetarchive/blob/master/CONTRIBUTING.rst
Your indentation is often wrong and you should not abbreviate index.
Also you should add spaces around operators.
If you had run the tests, Black would have complained about all those things except for the index part which it can't sensibly catch.
Using the index does not make too much sense in my opinion, because the collection could change at any time and using identifiers to mark the beginning and end would probably lead to more consistent results.
| @@ -33,6 +33,8 @@ | |||
| -R, --retries=<retries> Set number of retries to <retries> [default: 5]. | |||
| -I, --itemlist=<file> Download items from a specified file. Itemlists should | |||
| be a plain text file with one identifier per line. | |||
| -n, --start-idx=<n> Start immediately at item <n> | |||
There was a problem hiding this comment.
Change the long argument name to --start-index. The short from -n should probably be removed.
| @@ -33,6 +33,8 @@ | |||
| -R, --retries=<retries> Set number of retries to <retries> [default: 5]. | |||
| -I, --itemlist=<file> Download items from a specified file. Itemlists should | |||
| be a plain text file with one identifier per line. | |||
| -n, --start-idx=<n> Start immediately at item <n> | |||
| -m, --end-idx=<m> End download after item <m> | |||
There was a problem hiding this comment.
Change the long argument name to --end-index. The short from -m should probably be removed.
| @@ -128,6 +132,18 @@ def main(argv, session: ArchiveSession) -> None: | |||
| print(f'{exc}\n{printable_usage(__doc__)}', file=sys.stderr) | |||
| sys.exit(1) | |||
|
|
|||
| if args['--start-idx']: | |||
| start_idx = int(args['--start-idx'])-1 | |||
There was a problem hiding this comment.
Add spaces around all operators int(args['--start-index']) - 1. Also use four spaces for indentation as used everywhere else in the code ...
And if it should no be clear from the argument remark, also use start_index for the variable name instead of using idx.
| @@ -128,6 +132,18 @@ def main(argv, session: ArchiveSession) -> None: | |||
| print(f'{exc}\n{printable_usage(__doc__)}', file=sys.stderr) | |||
| sys.exit(1) | |||
|
|
|||
| if args['--start-idx']: | |||
| start_idx = int(args['--start-idx'])-1 | |||
| print(f'Starting download at collection item {start_idx+1}') | |||
| identifier = identifier.get('identifier') | ||
| if total_ids > 1: | ||
| item_index = f'{i + 1}/{total_ids}' | ||
| if end_idx != None and end_idx == i: |
There was a problem hiding this comment.
None is a singleton. Use is not None for the comparison. In this case end_index == i would also suffice as condition.
| ) | ||
| if _errors: | ||
| errors.append(_errors) | ||
| try: |
There was a problem hiding this comment.
The indentation in this whole section is wrong.
| ) | ||
| if _errors: | ||
| errors.append(_errors) | ||
| ##endif (start_idx) |
There was a problem hiding this comment.
Remove this superfluous line.
|
OK, I will look at making all of those changes and re-submit. In regards to marking beginning and end -- what identifier do you think might be best to use? I know little to nothing about how the IA uses metadata and so on; perhaps there's a specific one that will be constant even as a collection is updated? (I have already noticed a collection I was testing this on has grown since I began; but so far the older items hadn't moved around so resuming at a specific item number seemed to result in the same one each time.) |
|
As far as I know there is no marker which remains always constant in regard to the sequence. But I also think that it is close to impossible to cover the edge cases here. (e. g. an existing item could also just be replaced) I would think that using the item's identifier (i. e. the "name" of the item) would at least give a somewhat more reliable outcome when splitting a sequence. But every approach has its own issues. I guess your current approach is easier to use for your use case. |
|
What if you just used
And then when you resumed, you could do something like:
Alternatively, you could use |
Addition of
--start-idx=<n>and--end=idx=<m>options for cli download; this both allows downloading specific ranges of items within a collection, and speeds up resumption of incomplete collection downloads immensely, directing the iteration loop to skip (n-1) entries with--start-idxwithout checking them against locally-downloaded items.