2
\$\begingroup\$

I am trying to get all image urls which are valid and unique.To make sure i wont end up using the same image again i calculate hash of the image and store it in a text file and read the text file again.if there is any better way to do this, please let me know.Overall any speed optimisation would be really helpful, thanks in advance.

import multiprocessing as mp
import socket
# Set the default timeout in seconds
timeout = 20
socket.setdefaulttimeout(timeout)

from PIL import Image 
import hashlib
import os
import pickle

def getImages(val):
    # import pdb;pdb.set_trace()
    #Dowload images
    f = open('image_files.txt', 'a')
    k = open('image_hash.txt', 'a')    
    try:
        url=val # preprocess the url from the input val
        local=url.split('/')[-1] #Filename Generation From Global Varables And Rand Stuffs...
        urllib.request.urlretrieve(url,local)
        md5hash = hashlib.md5(Image.open(local).tobytes())
        image_hash = md5hash.hexdigest()
        k.write(image_hash+'\n')
        z = open('image_hash.txt', 'r')            
        a = z.readlines()
        image_hash_list = [c.strip('\n') for c in a]
        if image_hash not in image_hash_list:
            os.remove(local)
            f.write(url+'\n')
            print("DONE - " + url)
            return 1
        else:
            os.remove(local)
    except Exception as e:
        # print(e)
        print("CAN'T DOWNLOAD - " + url )
        return 0

files = "Identity.txt"
lst = list(open(files))
lst = [l.replace("\n", "") for l in lst]
lst = lst[::-1]
pool = mp.Pool(processes=12)
res = pool.map(getImages, lst)
print ("tempw")

Here identity.txt file looks like this,

http://url1.com
http://url2.com
...
\$\endgroup\$
  • \$\begingroup\$ 1) what is the point of if image_hash not in image_hash_list: condition? since you inevitably write image_hash into 'image_hash.txt', then - read from it and it will be there; 2) why not check if url is already in 'image_files.txt' beforehand ? 3) how does "Identity.txt" look like? \$\endgroup\$ – RomanPerekhrest Nov 11 at 19:28
  • \$\begingroup\$ nice catch, fixed it locally, identity.txt is a text file with all urls with duplicates and junk data \$\endgroup\$ – Ryan Nov 12 at 5:27
  • \$\begingroup\$ Consider storing your hashes in an sqlite3 database instead of a flat file. This is typically a more efficient and fast method of storing/retrieving data. \$\endgroup\$ – Chev_603 Nov 12 at 5:36
  • \$\begingroup\$ @Ryan, considering the above comments, would you mind to update your question (as you mentioned " fixed it locally") and post a short fragment of Identity.txt ? \$\endgroup\$ – RomanPerekhrest Nov 12 at 7:56
  • \$\begingroup\$ @RomanPerekhrest Hey i have made required edits.Thanks. \$\endgroup\$ – Ryan Nov 12 at 8:56
3
\$\begingroup\$

You should split your function up into smaller pieces. This way it gets a lot easier to reason about them individually, and to change them if needed.

You don't really need the image_hash.txt file, unless your number of URLs is more than a million or so. But if that is the case, you are probably better off using a database for this.

Since you are running this in parallel, your output is not deterministic. For two URLs pointing to the same image, the one that is determined to be the "true" URL depends on which is checked first, which may vary from run to run. This just sounds like bad design. Instead, just group image URLs by their image hashes. I opted to return the class name of the exception for invalid URLs, so you can find all of those grouped together as well (and still debug it if you have a NameError or TypeError somewhere in your code).

Instead of getting the filename from the URL and saving to that file, I would just keep the image in memory. This should be a lot faster than writing it to disk, just to immediately read it again for the hashing. Note that if a link to a very large file is passed, this might fill up your RAM (but it would also fill your disk in that case, and for RAM at least the process will be automatically killed by the OS, whereas for disk you won't notice until it is too late and have to do it manually). Fortunately PIL.Image.open can directly deal with a io.BytesIO object, so we don't need to do anything fancy. If you need to check images larger than your RAM, have a look at a previous revision of this answer, where I used a tempfile.SpooledTemporaryFile which gets written to disk in that case.

I personally prefer requests over urllib, since it covers all of the basic usage. Including raising an error if the connection fails, following redirects, etc.

from collections import defaultdict
import hashlib
import io
from itertools import tee
import multiprocessing
from PIL import Image
import requests


def get_hash(file):
    md5hash = hashlib.md5(Image.open(file).tobytes())
    return md5hash.hexdigest()

def process_url(url):
    try:
        r = requests.get(url, allow_redirects=True)
        r.raise_for_status()
        return get_hash(io.BytesIO(r.content))
    except Exception as e:
        return type(e).__name__

def get_urls(file_name):
    with open(file_name) as f:
        return {line.strip() for line in f}

def get_hashes(urls):
    n = multiprocessing.cpu_count() - 1
    with multiprocessing.Pool(processes=n) as pool:
        return pool.map(process_url, urls)

def invert_dict(d):
    d2 = defaultdict(set)
    for key, value in d.items():
        d2[value].add(key)
    return dict(d2)

def main():
    urls = get_urls('image_files.txt')
    hashes = dict(zip(urls, get_hashes(urls)))
    groups = invert_dict(hashes)
    print(groups)

if __name__ == "__main__":
    main()

For a file containing the following lines (our two avatars, yours with a slightly different URL as well, and mine with a misspelled URL), the result is as expected:

https://www.gravatar.com/avatar/8d627cb69cbceed874a485cd7cb3fa86?s=328&d=identicon&r=PG&f=1
https://www.gravatar.com/avatar/8d627cb69cbceed874a485cd7cb3fa86?s=328&d=identicon&r=PG
https://www.gravatar.com/avatar/b869ca5adc52b16ef56ab1fb6ed5fe97?s=328&d=identicon&r=PG
https://www.avatar.com/avatar/b869ca5adc52b16ef56ab1fb6ed5fe97?s=328&d=identicon&r=PG
{'242edcfd4ee09f9d734497a32c04fc06': {'https://www.gravatar.com/avatar/8d627cb69cbceed874a485cd7cb3fa86?s=328&d=identicon&r=PG&f=1', 
                                      'https://www.gravatar.com/avatar/8d627cb69cbceed874a485cd7cb3fa86?s=328&d=identicon&r=PG'},
 '3b7e2db9a0246462880635b9eeef45f3': {'https://www.gravatar.com/avatar/b869ca5adc52b16ef56ab1fb6ed5fe97?s=328&d=identicon&r=PG'},
 'OSError':                          {'https://www.avatar.com/avatar/b869ca5adc52b16ef56ab1fb6ed5fe97?s=328&d=identicon&r=PG'}}

This even has both the forward and backward dictionaries around for later, so if you need the hash of a URL, or the URL(s) of a hash, you can get both.

\$\endgroup\$
3
\$\begingroup\$

here's a few comments in no particular order.

Have a look over PEP8. It's Python's style guide which helps keep Python code consistent and readable. Things like naming functions which snake_case, constants as CAPS, proper spacing around operators, commenting style and placement, spacing between imports, etc.

It's recommended to use a context manager for opening files, this prevents them from being left open and the indentation makes it clear when a file is being used. You can open two files at once like so:

with open('file_one.txt', 'a') as file_one, open('file_two.txt', 'a') as file_two:
    ...

Instead of using return to handle errors, just raise the error itself. Catch the specific exception (ValueError, TypeError, etc (not Exception or worse BaseException)) then raise it with a message like so:

except TypeError:
    raise TypeError('Reason for failure message')

If you want to continue having caught the error, you can print the error message and continue instead:

except TypeError as exc:
    print(f'Reason for failure: {exc}; continuing ...')

A better way of storing lines from a file to a list would be to use readlines() and strip(). readlines() converts a file to a list where each item is a line in the file, and strip() can remove unwanted linebreaks and whitespace.

Wrap the code you're going to run with:

if __name__ == '__main__':
    ...

This stops any old code being executed if importing it as a module.

Why do you take val as an argument then immediately rename it to url? Just take url as the argument, it's more descriptive. You've also used url in your except block where it may not have been defined. It wouldn't fail in this example but it could have done if you wrote code like this elsewhere.

Your variable names could be much improved. No single letter variables! Make them descriptive and avoid putting their type in the name. k -> hash_file; image_hash_list -> image_hashes; etc.

You're importing pickle but not using it. You're also using urllib without importing it.

You're calling os.remove(local) regardless of the if condition. Put this outside the if check.

There are a few other nitpicks and I can't speak to what the code is actually doing as it's not something I'm familiar with, but I hope that helps.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.