3
\$\begingroup\$

This function takes as an argument a JSON file (could contain anything in JSON format, since I scrape hundreds of random pages) and returns a list of dictionaries where a URL is mapped to its corresponding headers, based on the extraction of headers using BeautifulSoup and a regex pattern.

I'm looking for suggestions regarding performance readability and clarity.

Following my 1st iteration I improved my code and here is the result:

import json
from tqdm import tqdm
import re
from bs4 import BeautifulSoup
import csv
import string

"Load HTML body, and fetch headers"
def get_headers_from_json(local_path):
    """
    The function takes a json file with html_body and returns a list of headers.
    It parses the titles, based on tags starting with 'h' + num.
    """
    data = json.loads(open(local_path).read())
    pattern = re.compile(r"^h[0-9]$")
    headers_urls = []
    printable = set(string.printable)
    for dict in tqdm(data):
        headers = []
        for val in dict.values():
            soup = BeautifulSoup(val, 'html.parser')
            url = dict.values()[0]
        for element in soup.find_all(pattern):
            element = element.get_text().strip().encode('utf-8')
            element = filter(lambda word: word in printable, element)  
            headers.append(element)
        cleaned_data = {"url": url, "headers": headers}
        headers_urls.append(cleaned_data)
    return headers_urls

Example of json input:

[["<body class=\" catalog-category-view categorypath-sale-html category-sale\">\n<script type=\"text/javascript\">\n//<![CDATA[\nif (typeof(Varien.searchForm) !== 'undefined') {\n    Varien.searchForm.prototype._selectAutocompleteItem = function(element) {\n        var link = element.down();\n        if (link && link.tagName == 'A') {\n            setLocation(link.href);\n        } else {\n            if (element.title){\n                this.field.value = element.title;\n            }\n            this.form.submit();\n        }\n    };\n    Varien.searchForm.prototype.initAutocomplete = function(url, destinationElement) {\n        new Ajax.Autocompleter(\n            this.field,\n            destinationElement,\n            url,\n            {\n                paramName: this.field.name,\n                method: 'get',\n                minChars: 2,\n                frequency: .3,\n                updateElement: this._selectAutocompleteItem.bind(this),\n                onShow : function(element, update) {\n                    if(!update.style.position || update.style.position=='absolute') {\n                        update.style.position = 'absolute';\n                        Position.clone(element, update, {\n                            setHeight: false,\n                            offsetTop: element.offsetHeight\n                        });\n                    }\n                    Effect.Appear(update,{duration:0});\n                }\n\n            }\n        );\n    };\n    Autocompleter.Base.prototype.markPrevious = function() {\n        if (this.index > 0) {\n            this.index--;\n        } else {\n            this.index = this.entryCount - 1;\n        }\n        var entry = this.getEntry(this.index);\n        if (entry.select('a').length === 0) {\n            this.markPrevious(); // Ignore items that don't have link\n        }\n    };\n    Autocompleter.Base.prototype.markNext = function() {\n        if (this.index < this.entryCount - 1) {\n
Improve this question
\$\endgroup\$
2
  • \$\begingroup\$ Can you add an example of the expected JSON data. I don't get it from the various for loops… \$\endgroup\$
    – 301_Moved_Permanently
    Jun 29, 2017 at 12:53
  • \$\begingroup\$ Sure @MathiasEttinger - will add it to the original post immediately. Thanks. \$\endgroup\$
    – oba2311
    Jun 29, 2017 at 12:57

2 Answers 2

Reset to default
4
\$\begingroup\$

There are multiple things you can do to speed things up:

  • faster JSON parsing - try out ujson or simplejson; or even simplejson on PyPy
  • use lxml instead of html.parser (requires lxml to be installed)

  • do not parse the complete HTML, parse only what you need with SoupStrainer:

    pattern = re.compile(r"^h[0-9]$")
parse_only = SoupStrainer(pattern)
soup = BeautifulSoup(data, "lxml", parse_only=parse_only)

    You can then do simple soup.find_all() (or via a shortcut - soup()) since you'll only have header elements in your HTML soup.

Here is a demonstration of what SoupStrainer would do:

In [1]: import re

In [2]: from bs4 import BeautifulSoup, SoupStrainer

In [3]: data = """
   ...: <body>
   ...:     <h1>Some Paragraph 1</h1>
   ...:     <div>
   ...:         <h2>Some Paragraph 2</h2>
   ...:     </div>
   ...:     
   ...:     <span>
   ...:         <div>
   ...:             <h3>Some Paragraph 3</h3>
   ...:         </div>
   ...:         <h4>Some Paragraph 4</h4>
   ...:     </span>
   ...:     <h5>Some Paragraph 2</h5>
   ...: </body>
   ...: """

In [4]: pattern = re.compile(r"^h[0-9]$")

In [5]: parse_only = SoupStrainer(pattern)

In [6]: soup = BeautifulSoup(data, "lxml", parse_only=parse_only)

In [7]: print(soup.prettify())
<h1>
 Some Paragraph 1
</h1>
<h2>
 Some Paragraph 2
</h2>
<h3>
 Some Paragraph 3
</h3>
<h4>
 Some Paragraph 4
</h4>
<h5>
 Some Paragraph 2
</h5>
Improve this answer
\$\endgroup\$
4
  • \$\begingroup\$ Thanks @alecxe . I read through bs4 documents but am still not sure why lxml would be better in this case than html.parser . Could you clarify that ? thanks. \$\endgroup\$
    – oba2311
    Jul 2, 2017 at 13:13
  • 1
    \$\begingroup\$ @oba2311 yes, sure, lxml is, generally, the fastest XML/HTML parser that you can use together with BeautifulSoup. You are basically gaining performance "for free" just by installing lxml and specifying it in the BeautifulSoup constructor without changing the rest of the code. Thanks. \$\endgroup\$
    – alecxe
    Jul 2, 2017 at 16:51
  • 1
    \$\begingroup\$ Thank you. Only one thing - lxml parser treats differently tags, so performance "for free" is not exactly 100% correct - is it? \$\endgroup\$
    – oba2311
    Jul 3, 2017 at 8:20
  • 1
    \$\begingroup\$ @oba2311 fair point; strictly speaking, html-parsing logic might differ from parser to parser, but, in reality, I stumble upon significant html parsing differences quite rarely. \$\endgroup\$
    – alecxe
    Jul 3, 2017 at 11:49
2
\$\begingroup\$

This part seems strange:

for val in dict.values():
    soup = BeautifulSoup(val, 'html.parser')
    url = dict.values()[0]
for element in soup.find_all(pattern):
    # ...

At the very least, calling dict.values() both as the thing to loop over and then calling it again inside the loop is redundant (although the performance hit is likely small). You should at least do this:

values = dict.values()
for val in values:
    soup = BeautifulSoup(val, 'html.parser')
    url = values[0]
for element in soup.find_all(pattern):
    # ...

Although that is still confusing. You loop over values, but in the end your soup variable is going to be populated based only upon the last value, while your url will be populated according to your first value. I'm guessing that is not quite what you are going for.

Without a better understanding of what exactly dict contains, my guess is that it will only ever contain one value, in which case you would probably be better off doing something like this:

values = dict.values()
if len( values ) != 1:
    raise ValueError( "Incorrect values" )

soup = BeautifulSoup( values[0], 'html.parser' )
url = values[0]

Therefore dropping the loop completely.

My only other suggestion would be an organizational one. Your function gets passed a filename, opens, reads (doesn't close), and then works with that. I think it would be better to just have it operate against a string, that way your usage can be more flexible. You can always add a function that reads the file, calls the parser, and then returns. The idea is basic separation of concerns: it is better to have one function that reads and another function that parsers, rather than one function that reads and parsers. The latter will cause you trouble when you need to operate on a string in memory, or elsewhere.

Improve this answer
\$\endgroup\$
2
  • \$\begingroup\$ Thanks @Conor. 1. Could you explain why performance would be better when iterating over values instead of dict.values()? 2. If you follow the code, you can tell that dict contains data which is a json. Example for the json is also attached to original post. 3. I will split the function to two. \$\endgroup\$
    – oba2311
    Jul 2, 2017 at 8:32
  • 1
    \$\begingroup\$ Regarding performance, the situation is complicated, but as a general rule of thumb there is a certain amount of overhead associated with calling a function in almost every programming language. This short article has some hard numbers for python: ilovesymposia.com/2015/12/10/the-cost-of-a-python-function-call The actual situation is very complicated, as many compilers optimize this when they can. However there is a good habit to get in: if you have data that never changes in your loop, store it in a variable once before hand: don't recalculate it each time. \$\endgroup\$
    – Conor Mancone
    Jul 2, 2017 at 10:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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