3
\$\begingroup\$

I have the following code which works fine but the execution time is more which I want to reduce. I guess this happens since each file is opened and then the operation is performed. Is there a way to open the file once, perform all operations, and then close it, which may improve the processing speed?

At a high level, the code is using Beautiful soup and finding tags and changing href, id attributes content to lowercase, change referenced file extension from xml to dita. Last piece of code changes the filenames to lowercase and file extension from xml to dita. I have added comments before each block to indicate the operation that the code is performing.

All the functions work as expected, but, the problem is with the processing time since every time file is opened and written for each function. I want to improve it by combining all the functions together so that the file is opened once, all changes are made at the same time, which may improve the performance. I tried to bring in all the functions together, but, the code did not work. I am new to Python, so do not have the expertise needed to achieve this.

import os
import glob
from bs4 import BeautifulSoup as bs

#Following code is to find fig, concept, table tags in all xml files and change id attribute to lowercase.

def lower_figcontab_id(file_path):
    with open(path, encoding="utf-8") as f:
        s = f.read()
    s = bs(s, "xml")
    fct = s.find_all(["fig", "concept", "table"])
    for i in fct:
        if "id" in i.attrs:
            i.attrs["id"] = i.attrs["id"].lower()
    s = str(s)
    with open(path, "w", encoding="utf-8") as f:
        f.write(s)

upper_directory = "C:/Users/sh001/Desktop/onemore/content" #add your directory path here
for dirpath, directories, files in os.walk(upper_directory):
    for files in glob.iglob('C:/Users/sh001/Desktop/onemore/content/**/*.xml', recursive=True): #finds all .xml files and updates references
        for fname in files:
            path = os.path.join(dirpath, files)
            lower_figcontab_id(path)

#Following code is to find image, xref, topicref tags and change href attribute to lowercase, and change .xml to dita file reference extension in href attribute.

def lower_topic_references(file_path):
    with open(path, encoding="utf-8") as f:
        s = f.read()
    s = bs(s, "xml")
    refs = s.find_all("topicref")
    for i in refs:
        if "href" in i.attrs:
            i.attrs["href"] = i.attrs["href"].replace("xml", "dita").lower()
    s = str(s)
    with open(path, "w", encoding="utf-8") as f:
        f.write(s)

upper_directory = "C:/Users/sh001/Desktop/onemore/content" #add your directory path here
for dirpath, directories, files in os.walk(upper_directory):
    #for files in glob.iglob('C:/Users/sh001/Desktop/newtest/**/*.xml', recursive=True): #finds all .xml files and updates references
     for files in glob.iglob('C:/Users/sh001/Desktop/onemore/content/**/*.ditamap', recursive=True): #finds all .dita files and updates references
      for fname in files:
                path = os.path.join(dirpath, files)
                lower_topic_references(path)

#Following code finds the image, xref, topicref tags and changes the case of href to lowercase.
def lower_file_references(file_path):
    with open(path, encoding="utf-8") as f:
        s = f.read()
    s = bs(s, "xml")
    imgs = s.find_all(["image", "xref", "topicref"])
    for i in imgs:
        if "href" in i.attrs:
            i.attrs["href"] = i.attrs["href"].lower()
    s = str(s)
    with open(path, "w", encoding="utf-8") as f:
        f.write(s)


upper_directory = "C:/Users/sh001/Desktop/onemore/content" #add your directory path here
for dirpath, directories, files in os.walk(upper_directory):
    for files in glob.iglob('C:/Users/sh001/Desktop/onemore/content/**/*.xml', recursive=True): #finds all .xml files and updates references
        #for files in glob.iglob('C:/Users/sh001/Desktop/newtest/**/*.dita', recursive=True): #finds all .dita files and updates references
     #if files.endswith(".xml") or files.endswith(".dita"):
            for fname in files:
                path = os.path.join(dirpath, files)
                lower_file_references(path)

#Following code finds xref tag and replaces .xml to .dita for href attribute.
def change_file_extension_in_references(file_path):
    with open(path, encoding="utf-8") as f:
        s = f.read()
    s = bs(s, "xml")
    ext = s.find_all(["xref"])
    for i in ext:
        if "href" in i.attrs:
            i.attrs["href"] = i.attrs["href"].replace(".xml",".dita")
    s = str(s)
    with open(path, "w", encoding="utf-8") as f:
        f.write(s)

upper_directory = "C:/Users/sh001/Desktop/onemore/content" #add your directory path here
for dirpath, directories, files in os.walk(upper_directory):
    for files in glob.iglob('C:/Users/sh001/Desktop/onemore/content/**/*.xml', recursive=True): #finds all .xml files and updates references
        for fname in files:
            path = os.path.join(dirpath, files)
            change_file_extension_in_references(path)


#Following code changes file extension from .xml to .dita and changes the case to lowercase.
path = "C:/Users/sh001/Desktop/onemore/content"

for dir,subdir,listfilename in os.walk(path):
    for filename in listfilename:
        new_filename = filename.replace(".xml",".dita").lower() 
        src = os.path.join(dir, filename) 
        dst = os.path.join(dir, new_filename) 
        os.rename(src,dst)
```
\$\endgroup\$
  • \$\begingroup\$ Please tell us more about what the code is supposed to do. The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How to Ask for examples, and revise the title accordingly. \$\endgroup\$ – Mast May 12 at 9:19
  • \$\begingroup\$ A data sample would help as well, please show an example of input file and what result you want. \$\endgroup\$ – Anonymous May 12 at 10:49
  • \$\begingroup\$ I will prepare a sample and share it soon. \$\endgroup\$ – Shilpa May 12 at 11:46
  • \$\begingroup\$ From the documentation of glob: "Note: Using the “**” pattern in large directory trees may consume an inordinate amount of time. " \$\endgroup\$ – David K May 12 at 12:36
  • \$\begingroup\$ What's going on with def lower_figcontab_id(file_path) when you don't use file_path anywhere else in the definition of the function? \$\endgroup\$ – David K May 12 at 12:40
1
\$\begingroup\$

I'm not surprised you had trouble modifying the code and making it work. It takes some careful thought to understand how it is working at all.

upper_directory = "C:/Users/sh001/Desktop/onemore/content" #add your directory path here
for dirpath, directories, files in os.walk(upper_directory):

Here, os.walk creates a generator that "visits" C:/Users/sh001/Desktop/onemore/content and every subdirectory of C:/Users/sh001/Desktop/onemore/content, recursively. The for ensures that the code following this line will execute once for each directory visited by the generator.

    for files in glob.iglob('C:/Users/sh001/Desktop/onemore/content/**/*.xml', recursive=True): #finds all .xml files and updates references

Here, glob.iglob returns a list containing the full paths of all *.xml files in C:/Users/sh001/Desktop/onemore/content and in all subdirectories of C:/Users/sh001/Desktop/onemore/content, recursively. The for ensures that the lines after it will be executed once for each filepath in the list.

Already I can see a reason why the script is so slow. The line starting for files in glob.iglob all by itself ensures that you will process every file in C:/Users/sh001/Desktop/onemore/content and all its subdirectories (recursively searched). The line before that line ensures that you won't just process the files once. You'll process all the files, and then you'll process them all again N more times, where N is the number of subdirectories of C:/Users/sh001/Desktop/onemore/content (recursively searched).

But didn't you need dirpath from os.walk in order to make the following work?

            path = os.path.join(dirpath, files)

Actually, you didn't. Since files is an absolute path to a file, join ignores dirpath and just returns files.

But back to your nested loops:

        for fname in files:

Here, files is a single string that is defined in one iteration of the previous for files. Since files is a string, for fname in files: iterates once for each letter in the string, setting fname to each letter in turn.

OK, so not only are you processing every file multiple times due to iterating over os.walk, you are also multiplying the multiple times by the number of characters in the full file path to each file. That is, if C:/Users/sh001/Desktop/onemore/content has just 2 subdirectories that have 2 subdirectories each (and no more), that's 7 directories total; and if the average length of the full path to each file (including the initial part of the path, C:/Users/sh001/Desktop/onemore/content) is 50 characters, you are processing every single file 350 times. No wonder it's slow.

(It's possible I've made a mistake somewhere in this analysis. Honestly, I didn't check very carefully, because code of such poor quality is not worth the trouble. You should know that nesting three different kinds of for loops like this is a terrible code smell. Just don't do it.)

When you're learning how to process a lot of files using iterators like these, it's helpful to insert some debug printout in the early versions of your script, printing out the name of the file just before you process that file. In this case, with multiple loops, I would just debug the first loop to start with (because I would only write the first loop to start with and would wait until I was sure it worked right before writing any other processing). For example, instead of just calling lower_figcontab_id(path), you could have

            print(path)
            lower_figcontab_id(path)

You might want to take out the print once everything is working OK. Or maybe not--it can be helpful to have confirmation in the console output that your script is working.


I think you should start over, though you can salvage some bits and pieces from this script (such as the operations you perform on the BeautifulSoup objects). A good basic structure would be to write a loop of some kind--using os.walk is fine, or glob.iglob, but not both--that will list the full path to each *.xml file exactly once. Once you have such a full path in a variable named path, this part of your code makes a BeautifulSoup object s from it:

    with open(path, encoding="utf-8") as f:
        s = f.read()
    s = bs(s, "xml")

Do all the operations you want on s. It's helpful to have a function for each transformation, as you do, but rather than take a path as the argument to the function it's better to take s as the argument to the function and return the modified copy of s back to the caller when you're done. After all that you can use the code you've already written to write the contents of s to the file.


To avoid making such a mess in the future, write your scripts incrementally. Get a few lines that do some part of the job and test that they actually do what you think (and not a lot more than you think or a lot less). Temporary print statements can help you keep track of your failures and successes. When you have pieces that work you can put subroutines in them or use them as subroutines for other pieces.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks David. The analysis you have made is right about the code. Since the files are being opened multiple times, the execution time is getting impacted. Initially, there was only os.walk but that started manipulating the graphic files too which became unusable, hence I had to include glob function to select the file type to parse. This may have slowed down the execution time. Any suggestion to select file type without using glob function? I am very new to scripting, hence don't know much about what would be the best method to use. \$\endgroup\$ – Shilpa May 15 at 5:05
  • \$\begingroup\$ I tried using if files.endswith('.xml'): and got the error -> AttributeError: 'list' object has no attribute 'endswith' \$\endgroup\$ – Shilpa May 15 at 5:23
  • \$\begingroup\$ I tried using if fname.endswith('.xml'): and got the error -> join() argument must be str or bytes, not 'list' \$\endgroup\$ – Shilpa May 15 at 5:43
  • \$\begingroup\$ I was able to fix all errors and also improve the execution time. Before the script would take 7 minutes for 2 folders to execute all the functions, now, it takes less than a minute to parse through 2 folders and make all the changes. Thanks for the tip on not including os.walk and glob functions together. \$\endgroup\$ – Shilpa May 15 at 5:58
  • \$\begingroup\$ Improved piece of code (added this to all functions) -> for fname in files: if fname.endswith(".xml"): path = os.path.join(dirpath, fname) lower_figcontab_id(path) \$\endgroup\$ – Shilpa May 15 at 5:58

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.