4
\$\begingroup\$

I use this script to get a list of all my photos in a folder and its subfolders. However, the program seems to be really slow.

There are 50,000 .jpg pictures in this folder and subfolders. I can reduce the for loops but the program still runs more or less at the same speed.

I am open to using lambda as well but would like to use Python's basic functions at the best possible speed. Can anyone suggest an improvement?

import os
from fnmatch import fnmatch
import sys

root = "C:\\Users\\Agartha\\Desktop\\photos"
pattern = "*.jpg"

with open("./files\\list.txt", "w") as a:
    for path, subdirs, files in os.walk(root):
       for filename in files:
            if fnmatch(filename,pattern):
                a.write(str(os.path.join(filename)) + '\n')
\$\endgroup\$
  • 1
    \$\begingroup\$ Why do you os.path.join on a single argument? Did you meant to write os.path.join(path, filename)? \$\endgroup\$ – Mathias Ettinger Mar 12 '18 at 11:05
  • \$\begingroup\$ did you consider using regex for the pattern matching? \$\endgroup\$ – A. Romeu Mar 12 '18 at 12:46
  • 1
    \$\begingroup\$ @A.Romeu fnmatch actually convert the pattern to a regex under the hood. See the source code, \$\endgroup\$ – Mathias Ettinger Mar 13 '18 at 9:12
2
\$\begingroup\$

The main problem is that your method relies heavily on your hard drive. The process needs to recursively look inside every sub-folder, which takes time, especially with 50000 files.

Windows search is much faster because it indexes the files and doesn't scan the whole drive for each query.

If using Python3 is a possibility, the only thing I'd change in your code is to use glob and replace the loops with:

import glob
glob.glob("C:\\Users\\Agartha\\Desktop\\photos\\**\\*.jpg", recursive=True)
\$\endgroup\$
  • \$\begingroup\$ if he uses Python 3, I would use pathlib.Path.glob instead of glob.glob \$\endgroup\$ – Maarten Fabré Mar 12 '18 at 15:03
  • \$\begingroup\$ @MaartenFabré: It really depends what OP wants to do with it. If it's just to write filenames into a file, glob.glob will be more than enough. For anything more complex, pathlib is great indeed. \$\endgroup\$ – Eric Duminil Mar 12 '18 at 15:08
2
\$\begingroup\$

Here you do 4 things at the same time. Your are

  1. Walking over all the files of the directory
  2. Comparing the filename to a pattern
  3. Formatting the filename
  4. Appending the filename to a file

To find out what is the slower step, you should decouple this

def find_files(root):
    for path, subdirs, files in os.walk(root):
        for filename in files:
            yield filename

def filter_filename(files, pattern):
    for filename in files:
        if fnmatch(filename,pattern):
            yield filename

def format_filenames(files, root):
    for filename in files:
        yield str(os.path.join(filename)) + '\n'

def writelines(out_file, files):
    for filename in files:
        out_file.write(filename)
    # or:
    # out_file.writelines(files)

def main(root, pattern, filename_out):
    files = find_files(root)
    files_filtered = filter_filename(files, pattern)
    files_formatted = format_filenames(files, root)
    with open(filename_out, 'w') as out_file:
        writelines(out_file, files_formatted)

Then Afterwards, there are 4 things you can time individually to find out the largest culprit in time consumption and work on that one:

  1. files = list(find_files(root))
  2. files_filtered = list(filter_filename(files, pattern))
  3. files_formatted = list(format_filenames(files, root))
  4. with open(filename_out, 'w') as out_file: writelines(out_file, files_formatted)
\$\endgroup\$
2
\$\begingroup\$

For starter, you could organize your code into functions and use the if __name__ == '__main__' guard.

You could also change the construct for loop + if fnmatch.fnmatch into for … in fnmatch.filter().

This would yield something like:

import os
import fnmatch

def filter_files(path='~/Desktop', filter_pattern='*.jpg'):
    for root, _, filenames in os.walk(os.path.expanduser(path)):
        for filename in fnmatch.filter(filenames, filter_pattern):
            yield os.path.join(root, filename)


def write_to_file(iterable, filename='files/list.txt'):
    with open(filename, 'w') as output:
        for element in iterable:
            output.write(element)
            output.write('\n')


if __name__ == '__main__':
    pictures = filter_files('~/Desktop/photos')
    write_to_file(pictures)

However, this code can be greatly improved if you switch to using Python 3.5+ as the recursive filtering is exactly implemented in glob.glob:

import glob


def write_to_file(iterable, filename='files/list.txt'):
    with open(filename, 'w') as output:
        for element in iterable:
            output.write(element)
            output.write('\n')


if __name__ == '__main__':
    pattern = 'C:\\Users\\Agartha\\Desktop\\photos\\**\\*.jpg'
    pictures = glob.iglob(pattern, recursive=True)
    write_to_file(pictures)

You could still maintain the filter_files function for easier interface:

import glob
from os.path import expanduser, join as path_join


def filter_files(path='~/Desktop', filter_pattern='*.jpg'):
    pattern = path_join(expanduser(path), '**', filter_pattern)
    yield from glob.iglob(pattern, recursive=True)


def write_to_file(iterable, filename='files/list.txt'):
    with open(filename, 'w') as output:
        for element in iterable:
            output.write(element)
            output.write('\n')


if __name__ == '__main__':
    pictures = filter_files('~/Desktop/photos')
    write_to_file(pictures)
\$\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.