This seems like a cool project, and a nice first introduction to web scraping! I'll cover some general advice first, and then address your main concern: speed.
General
You use two spaces for indentation. PEP-8 recommends 4 spaces (most code editors have the option to automatically convert tabs to spaces, which I highly recommend).
While on the subject, PEP-8 also recommends two blank lines between top-level functions.
In general, the use of globals is discouraged. There are some exceptions:
- Global constants are okay;
- Under certain circumstances, avoiding global state would result in unacceptably complicated code.
However, neither of these apply here. Using global, non-const variables makes your code harder to debug, and is a burden for anyone reading / updating it in the future. You can avoid global state here easily by passing variables as parameters.
In my opinion, show_logo() is a bit overkill. I'd make it a constant instead (LOGO).
The while loop in handle_menu() can be improved:
while True:
try:
global n_of_comics
n_of_comics = int(input(">> ").strip())
There's no need to call str.strip() before passing the return value to int.
except ValueError:
print("Error: incorrect value. Try again.")
continue
The error message is a bit vague. Why is the value incorrect? Maybe something like 'Error: expected a number. Try again.' would fit better.
if n_of_comics > len(found_comics) or n_of_comics < 0:
print("Error: incorrect number of comics to download. Try again.")
continue
This error message is quite clear :)
elif n_of_comics == 0:
sys.exit()
else:
break
You don't need the else clause here, since the elif clause stops the program if executed. In fact, you can simply return n_of_comics there. Putting this all together:
while True:
try:
global n_of_comics
n_of_comics = int(input(">> "))
except ValueError:
print("Error: expected a number. Try again.")
continue
if n_of_comics > len(found_comics) or n_of_comics < 0:
print("Error: incorrect number of comics to download. Try again.")
continue
elif n_of_comics == 0:
sys.exit()
return n_of_comics
Your 'main routine' isn't encapsulated in a function, which means it's hard to test / explore functions individually from a Python interactive session (or from a separate file). I suggest putting all this top level code into a main() function:
def main():
show_logo()
all_comics = fetch_comic_archive()
found_comics = filter_comic_archive(all_comics)
handle_menu()
start = time.time()
for link in generate_comic_link(found_comics, n_of_comics):
print("Downloading: {}".format(link))
move_to_dir(DEFAULT_DIR_NAME)
url = grab_image_src_url(link)
download_image(url)
end = time.time()
print("Successfully downloaded {} comics in {:.2f} seconds.".format(n_of_comics, end - start))
You can then check if __name__ == "__main__", to run main if the script is called as the main program (see 'What does if __name__ == "__main__" do?'):
if __name__ == "__main__":
main()
I don't get the point of move_to_dir(). It handles the case where the current working directory has been changed, and it's called for each comic to download. That seems pretty pointless to me. Instead, I would only create the directory once, in main():
DEFAULT_DIR_NAME = "poorly_created_folder"
COMICS_DIRECTORY = os.path.join(os.getcwd(), DEFAULT_DIR_NAME)
...
def download_image(link):
...
with open(os.path.join(COMICS_DIRECTORY, file_name), "wb") as file:
...
...
def main():
...
try:
os.mkdir(COMICS_DIRECTORY)
except OSError as exc:
sys.exit("Failed to create directory (errno {})".format(exc.errno))
# `sys.exit` will write the message to stderr and return with status code 1
generate_comic_link() is superfluous. The following functions all do the same:
def generate_comic_link(array, num):
# Using itertools.islice()
for link in itertools.islice(array, 0, num):
yield link
def generate_comic_link2(array, num):
# Using slicing
for link in array[:num]:
yield link
def generate_comic_link3(array, num):
# Using slicing with yield from
yield from array[:num]
itertools.islice() is overkill (and a little harder to read). Since generate_comic_link3() is a one-liner, you can probably get rid of the function altogether, iterating over the URLs directly using slicing.
A bit of a nitpick, but req = <requests>.get(<url>) is wrong. requests.get doesn't return the request, it returns the response. Thus, response = <requests>.get(<url>) makes more sense.
Some of your variables could (should) be constants:
url = 'http://www.poorlydrawnlines.com/archive/'
# Might be
ARCHIVE_URL = "http://www.poorlydrawnlines.com/archive/"
pattern = re.compile(r'http://www.poorlydrawnlines.com/comic/.+')
# Might be
COMIC_PATTERN = re.compile(r"http://www.poorlydrawnlines.com/comic/.+")
(I prefer double quotes, as you might be able to tell)
In fetch_comic_archive():
all_links = []
for link in soup.find_all('a'):
all_links.append(link.get('href'))
return all_links
... might be a one-liner:
return [link.get("href") for link in soup.find_all("a")]
In filter_comic_archive():
filtered_links = [i for i in archive if pattern.match(i)]
return filtered_links
There's no need for the intermediate variable.
Now for the actual challenge: improving performance.
You don't need asynchronous lxml, threads will do just fine here!
By wrapping the relevant code into a function, we can spawn any amount of threads to do the work:
import threading
def download_comic(link):
print("Downloading: {}".format(link))
move_to_dir(DEFAULT_DIR_NAME)
url = grab_image_src_url(link)
download_image(url)
...
def main():
...
for link in generate_comic_link(found_comics, n_of_comics):
thread = threading.Thread(target=download_comic, args=(link,))
thread.start()
thread.join()
# Join the last thread to make sure all comics have been
# downloaded before printing the time difference
...
Rewrite
Putting everything together (I've changed names, reordered some functionality and added comments explaining certain changes):
import time
import os
import sys
import re
import threading
# PEP-8 recommends a blank line in between
# stdlib imports and third-party imports.
import requests
# Importing `requests` *and* `get` from `requests` is confusing
from bs4 import BeautifulSoup as bs
DEFAULT_DIR_NAME = "poorly_created_folder"
COMICS_DIRECTORY = os.path.join(os.getcwd(), DEFAULT_DIR_NAME)
LOGO = """
a Python comic(al) scraper for poorlydwarnlines.com
__
.-----.-----.-----.----.| |.--.--.
| _ | _ | _ | _|| || | |
| __|_____|_____|__| |__||___ |
|__| |_____|
__ __ __
.--.--.--.----.|__| |_| |_.-----.-----.
| | | | _|| | _| _| -__| |
|________|__| |__|____|____|_____|__|__|
.-----.----.----.---.-.-----.-----.----.
|__ --| __| _| _ | _ | -__| _|
|_____|____|__| |___._| __|_____|__|
|__|
version: 0.2 | author: baduker | https://github.com/baduker
"""
ARCHIVE_URL = "http://www.poorlydrawnlines.com/archive/"
COMIC_PATTERN = re.compile(r"http://www.poorlydrawnlines.com/comic/.+")
def download_comics_menu(comics_found):
print("\nThe scraper has found {} comics.".format(len(comics_found)))
print("How many comics do you want to download?")
print("Type 0 to exit.")
while True:
try:
comics_to_download = int(input(">> "))
except ValueError:
print("Error: expected a number. Try again.")
continue
if comics_to_download > len(comics_found) or comics_to_download < 0:
print("Error: incorrect number of comics to download. Try again.")
continue
elif comics_to_download == 0:
sys.exit()
return comics_to_download
def grab_image_src_url(url):
response = requests.get(url)
soup = bs(response.text, "html.parser")
for i in soup.find_all("p"):
for img in i.find_all("img", src=True):
return img["src"]
def download_and_write_image(url):
# `download_and_write_image` is a bit more accurate, since
# it also writes the image to the disk
file_name = url.split("/")[-1]
with open(os.path.join(COMICS_DIRECTORY, file_name), "wb") as file:
response = requests.get(url)
# Replced `get` with `requests.get`
file.write(response.content)
def fetch_comics_from_archive():
# Merged `fetch_comic_archive` and `filter_comic_archive`
# into a single function
response = requests.get(ARCHIVE_URL)
soup = bs(response.text, "html.parser")
comics = [url.get("href") for url in soup.find_all("a")]
return [url for url in comics if COMIC_PATTERN.match(url)]
def download_comic(url):
print("Downloading: {}".format(url))
url = grab_image_src_url(url)
download_and_write_image(url)
def main():
print(LOGO)
comics = fetch_comics_from_archive()
comics_to_download = download_comics_menu(comics)
try:
os.mkdir(DEFAULT_DIR_NAME)
except OSError as exc:
sys.exit("Failed to create directory (errno {})".format(exc.errno))
start = time.time()
for url in comics[:comics_to_download]:
thread = threading.Thread(target=download_comic, args=(url,))
thread.start()
thread.join()
end = time.time()
print("Successfully downloaded {} comics in {:.2f} seconds.".format(
comics_to_download, end - start)
)
if __name__ == "__main__":
main()
Results
Non-threaded:
The scraper has found 957 comics.
How many comics do you want to download?
Type 0 to exit.
>> 6
Downloading: http://www.poorlydrawnlines.com/comic/new-phone/
Downloading: http://www.poorlydrawnlines.com/comic/new-things/
Downloading: http://www.poorlydrawnlines.com/comic/return-to-nature/
Downloading: http://www.poorlydrawnlines.com/comic/phone/
Downloading: http://www.poorlydrawnlines.com/comic/stars/
Downloading: http://www.poorlydrawnlines.com/comic/big-dreams/
Successfully downloaded 6 comics in 37.13 seconds.
Threaded:
The scraper has found 957 comics.
How many comics do you want to download?
Type 0 to exit.
>> 6
Downloading: http://www.poorlydrawnlines.com/comic/new-phone/
Downloading: http://www.poorlydrawnlines.com/comic/new-things/
Downloading: http://www.poorlydrawnlines.com/comic/return-to-nature/
Downloading: http://www.poorlydrawnlines.com/comic/phone/
Downloading: http://www.poorlydrawnlines.com/comic/stars/
Downloading: http://www.poorlydrawnlines.com/comic/big-dreams/
Successfully downloaded 6 comics in 7.07 seconds.
If you run the rewritten code, you may notice the program takes a couple of seconds to shut down after (reportedly) having downloaded all images. This is because the last thread started doesn't necessarily end last (that's the whole point of threads, after all!). To avoid this, and to get rid of some of the boilerplate code, we can use ThreadPoolExecutor.map() and ThreadPoolExecutor.shutdown().
I've created a GitHub gist here which uses ThreadPoolExecutor together with requests.Session, which reuses the underlying TCP connection, potentially resulting in even better performance.
