Tiny image scraper for xkcd.com

Question

This came out on the spur of the moment, as a quick and dirty tool to have the job done.

This is a simple image scraper for immensely popular and legendary comic website inspired by a Python Easter Egg.

For those who don't know it, run your Python interpreter and type import antigravity and hit Enter. :)

As for the code below, I'd appreciate any feedback, particularly in regards to threading, as I'm new to this.

#!/usr/bin/python3

import os
import sys
import time
import threading
from pathlib import Path
from shutil import copyfileobj


import requests
from lxml import html


BASE_URL = "https://www.xkcd.com/"
ARCHIVE = "https://www.xkcd.com/archive"
SAVE_DIRECTORY = Path('xkcd_comics')
LOGO = """
       _           _                      
 tiny | |  image  | | downloader for
 __  _| | _____ __| |  ___ ___  _ __ ___  
 \ \/ / |/ / __/ _` | / __/ _ \| '_ ` _ \ 
  >  <|   < (_| (_| || (_| (_) | | | | | |
 /_/\_\_|\_\___\__,_(_)___\___/|_| |_| |_|
 version 0.1
"""


def show_logo():
    print(LOGO)


def fetch_url(url: str) -> requests.Response:
    return requests.get(url)


def head_option(values: list) -> str:
    return next(iter(values), None)


def get_penultimate(url: str) -> int:
    page = fetch_url(url)
    tree = html.fromstring(page.content)
    newest_comic = head_option(
        tree.xpath('//*[@id="middleContainer"]/a[1]/@href'))
    return int(newest_comic.replace("/", ""))


def get_images_from_page(url: str) -> str:
    page = fetch_url(url)
    tree = html.fromstring(page.content)
    return head_option(tree.xpath('//*[@id="comic"]//img/@src'))


def get_number_of_pages(latest_comic: int) -> int:
    print(f"There are {latest_comic} comics.")
    print(f"How many do you want to download? Type 0 to exit.")
    while True:
        try:
            number_of_comics = int(input(">> "))
        except ValueError:
            print("Error: Expected a number. Try again.")
            continue
        if number_of_comics > latest_comic or number_of_comics < 0:
            print("Error: Incorrect number of comics. Try again.")
            continue
        elif number_of_comics == 0:
            sys.exit()
        return number_of_comics


def clip_url(img: str) -> str:
    return img.rpartition("/")[-1]


def make_dir():
    return os.makedirs(SAVE_DIRECTORY, exist_ok=True)


def save_image(img: str):
    comic_name = clip_url(img)
    print(f"Downloading: {comic_name}")
    f_name = SAVE_DIRECTORY / comic_name
    with requests.get("https:" + img, stream=True) as img, open(f_name, "wb") \
            as output:
        copyfileobj(img.raw, output)


def show_time(seconds: int) -> int:
    minutes, seconds = divmod(seconds, 60)
    hours, minutes = divmod(minutes, 60)
    time_elapsed = f"{hours:02d}:{minutes:02d}:{seconds:02d}" 
    return time_elapsed


def get_xkcd():
    show_logo()
    make_dir()

    collect_garbage = []
    latest_comic = get_penultimate(ARCHIVE)
    pages = get_number_of_pages(latest_comic)

    start = time.time()
    for page in reversed(range(latest_comic - pages + 1, latest_comic + 1)):
        print(f"Fetching page {page} out of {latest_comic}")
        try:
            url = get_images_from_page(f"{BASE_URL}{page}/")
            thread = threading.Thread(target=save_image, args=(url, ))
            thread.start()
        except (ValueError, AttributeError, requests.exceptions.MissingSchema):
            print(f"WARNING: Invalid comic image source url.")
            collect_garbage.append(f"{BASE_URL}{page}")
            continue
    thread.join()
    end = time.time()

    print(f"Downloaded {pages} comic(s) in {show_time(int(end - start))}.")

    if len(collect_garbage) > 0:
        print("However, was unable to download images for these pages:")
        print("\n".join(page for page in collect_garbage))


def main():
    get_xkcd()


if __name__ == '__main__':
    main()

Answer 1

Stop hammering XKCD server

• You basically wrote a denial-of-service attack, since you try to connect as fast and as often as possible to the server.

• By sharing this code on CodeReview, it becomes a distributed denial-of-service.

Just in case it wasn't clear : don't do that.

I suppose the load of XKCD servers is high enough that your code won't make any noticeable difference. Still, the primary goal of your code should be to stay under the radar. Seen from outside, there shouldn't be any difference between launching your script and casually browsing https://xkcd.com/.

It isn't just as a courtesy to fellow developers : you might be banned by the remote servers if you send too many requests too fast.

Solutions

• Wait at least one second before downloading any picture. The desired pause might be defined as Crawl-delay in robots.txt.
• Remove multi-threading or at least limit the number of threads.
• Don't download a file again if it is already in SAVE_DIRECTORY. You can check if it's here, if its size is larger than 0 or if it is indeed a PNG file.

Unrelated note

You know the id for each comic. You should probably write it in the filename : geologic_time.png could be called 2187_geologic_time.png or 02187_geologic_time.png.

This way, comics are sorted chronologically inside SAVE_DIRECTORY.

Answer 2

Globals

As it is, there would be an advantage to LOGO being a local instead of a global - it's only used by show_logo, and moving it there would clean up the global namespace.

That said (and as others have pointed out), it's fairly common to see stuff like this at the top of Python files in global scope. However, the larger issue is that if you move it to local scope, you have to get clever with indentation. There are no great solutions to this - either you have to de-indent all but the first line, which is ugly; or you have to post-process the string to remove all whitespace at the beginning of each line. So this one is kind of a wash.

Base URLs

You correctly saved a base URL, but then didn't use it in the correct contexts. Particularly problematic:

ARCHIVE = "https://www.xkcd.com/archive"

This ignores the BASE_URL entirely, when it shouldn't.

fetch_url is currently useless - it doesn't add anything to requests.get. You could make it useful by making the argument a path relative to the base path.

with requests.get("https:" + img
# ...
url = get_images_from_page(f"{BASE_URL}{page}/")

Naive string concatenation is not the right thing to do, here. Python has a full-featured urllib to deal with URL parsing and construction.

show_time

divmod on a numeric time interval is not the right thing to do. Use datetime.timedelta.

Answer 3

for page in reversed(range(latest_comic - pages + 1, latest_comic + 1)):
    print(f"Fetching page {page} out of {latest_comic}")
    try:
        url = get_images_from_page(f"{BASE_URL}{page}/")
        thread = threading.Thread(target=save_image, args=(url, ))
        thread.start()
    except (ValueError, AttributeError, requests.exceptions.MissingSchema):
        print(f"WARNING: Invalid comic image source url.")
        collect_garbage.append(f"{BASE_URL}{page}")
        continue
thread.join()

Here you create several threads that download the pages. There are at least two problems with this code:

1. You create a number of threads, but only join() the last one you created. There is no guarantee that all threads have finished before the last one does. Maintain a list of your threads.
2. No rate limit. If you try to download 100 pages, it will try to do all 100 simultaneously. That is not a good idea. Only create a limited amount of threads at a time.

Answer 4

As mentioned in a comment by Thomas, another option is to use XKCD's JSON interface rather than scraping HTML:

import requests, time, tempfile, os.path
from shutil import copyfileobj

path = tempfile.mkdtemp()
print(path)
f_name = "{num:04d}-{safe_title}.{ext}"
current_comic = requests.get("https://xkcd.com/info.0.json").json()

# Iterates over numbers from comic 1 to the comic before current
for n in range(1,current_comic["num"]):
    comic_req = requests.get("https://xkcd.com/{}/info.0.json".format(n))
    # if status code is 2**
    if comic_req.status_code <= 299:
        comic = comic_req.json()
        comic["ext"] = comic["img"][-3:]
        fn = f_name.format(**comic)

        img = requests.get(comic_req.json()["img"], stream=True)
        with open(os.path.join(path, fn), "wb") as output:
            copyfileobj(img.raw, output)
        img.close()

        print("Saved {}".format(os.path.join(path, fn)))

Answer 5

As for the code below, I'd appreciate any feedback, particularly in regards to threading

You might want to use a ThreadPoolExecutor to manage your threads. This approach has two advantages:

• The executor can be used as a context manager to make sure all the threads are joined.
• It lets you limit the number of threads in the thread pool.

Example:

    with ThreadPoolExecutor(max_workers=8) as executor:
        for page in reversed(range(latest_comic - pages + 1, latest_comic + 1)):
            print(f"Fetching page {page} out of {latest_comic}")
            try:
                url = get_images_from_page(f"{BASE_URL}{page}/")
                executor.submit(save_image, url)
            except (ValueError, AttributeError, requests.exceptions.MissingSchema):
                print(f"WARNING: Invalid comic image source url.")
                collect_garbage.append(f"{BASE_URL}{page}")

Answer 6

Not a problem in terms of functionality (seems to have been covered pretty well already) but in terms of clarity/readability:

Misleading name get_penultimate

It appears to functionally be get_last (get_ultimate, if you will), so there is a clear mismatch between this name and the implemented functionality. The functionality makes sense, so the name appears to be wrong.
To explicitly point out the mismatch: penultimate means second to last.

Answer 7

Why have main() be a single line calling a single function? Either rename main() to get_xkcd() or just call get_xkcd().

Also, maybe use BeautifulSoup instead of lxml.