Remove HTML format from io.ascii guess list - #3693

There is a good chance that this will resolve #3280 as well.

I found it a little surprising that BeautifulSoup gets so far into a vanilla .csv file without bailing since it doesn't look like an HTML file. Reading the HTML spec (http://www.w3.org/TR/html-markup/documents.html#syntax-document-html), if you ignore requirement to have a doctype declaration (which I suspect BeautifulSoup does in the interest of being "flexible") any old text file could be considered a valid HTML file, and so it probably does end up reading the whole file. And perhaps going along for so many bytes without actually seeing any elements is why it crashes (though that's a totally wild guess). In any event, a vanilla .csv is probably considered a real outlier for an XML parser.

In the context of what io.ascii considers an HTML table though, we could probably consider a file "not an HTML" table if it doesn't start with the following grammar:

(BYTE_ORDER_MARKER)? (WHITESPACE)* (LESS_THAN)

Rather than completely disabling the HTML parser altogether, you could read the first few bytes to see if it looks like an HTML table and if not, just bail out.

I think this is too heavy-handed, since it doesn't affect all users. I like @mdboom's suggestion as a starter, but in the meantime I'd rather not do anything too heavy-handed unless someone with the debugging skills can reproduce this and figure out what's going on.

It also sounds most likely like a bug in the libxml2.2 that is built for OSX on some versions (who knows?) of Anaconda.

I'm -1 on @mdboom's suggestion. At least for the LaTeX Reader (which is a similar case in the sense that the user will always know that the format is html or latex as opposed to the more generic ascii formats) it is very valuable that the reader works on snippets taken from a LaTeX file. We could fail the LaTeX Reader for all files that don't contain e.g. \begin{document}, but we don't because it allows the user to read snippets (e.g. cut and past table 5 from a lengthy article into a new file and read that).
Similarly, html files can contain many table elements (so I might want ot cut and paste just one without worrying that my snippet is not valid html itself), or the html table could have been generated by some other tool for inclusion into an html template.

Maybe we can have the requirement from @mdboom only when guessing.

I don't know how many people use cut and paste snippets as I described above, but in that case it would be handy if guessing worked for those. You might not be able to use the filename as identifier in this case. On the other hand, I don't think many people even know that we have an interface to read html tables and I guess most would not notice even if we removed the html reader entirely...

How about a solution similar to @mdboom, where we preprocess (just for guessing) a file for the html reader and require that re.match("<tr>[\n\t\s]*<td>" (or a similar regexp) returns at least one match?
If it matches, it goes into the html reader, which can still fail if it's not valid html.

@hamogu: Your suggestion is even more exclusive than mine, so I don't understand why you're not in favor of mine, but in favor of yours. I wasn't suggesting that we require an tag or a doctype decl or anything. Only that it starts with a <. I think HTML tables are pretty easy to identify relative to LaTeX ones.

@mdboom Sorry, I may have misunderstood your suggestion. I thought you wanted to require that the tag pattern matches at the beginning of a file. If I copy & paste a couple of lines from the middle of the html file, it might start with free-form text. However, I'm just saying what I think would be useful, I did not actually test if beautiful soup would parse such files that are certainly not valid html.

@hamogu: Just to be clear -- I'm not opposed to your even more specific regex -- but ideally we wouldn't have to read too much of the file before bailing, just for efficiency's sake.

Maybe something like < \s* tr in the first N characters (with N something like 10000?)? Or look for />?

Or plan B which is to pass the first N characters to the parser and see if it detects any HTML elements. This presumes that most parsers are pretty liberal and will come up with something, even if there are no ending tags.

As I mentioned, a reader has access to a guessing flag that let's it know that it is being called as part of a read with guessing. In that case we can afford to be stricter, so only in that case would the pre-detection test be applied. Then if the user has a file that doesn't parse in guess mode they can explicitly say format='html'.

What Mike is suggesting is in fact exactly the same thing libxml is doing. If it fails to find the start of an XML/HTML tag it goes into a routine that parses it as a CDATA block. That code has a fair bit of buffer manipulation chicanery that I don't fully understand yet, but its buffer manipulation is definitely where the segfault is occurring. By making that check for < ourselves we avoid that pain and don't even have to go into BeautifulSoup at all (or libxml by extension).

I agree with @hamogu that we want the following to still work:

In [1]: from astropy.io import ascii

In [2]: text = 'asdf asdf asdf \n asdf \n <table><tr><td>1</td> <td>hello</td></tr></table> junk etc'

In [3]: ascii.read(text)
Out[3]: 
<Table masked=False length=1>
 col1   col2  
int64 string40
----- --------
    1    hello

This may be a naive question, but is it possible for the first character of an HTML file (excluding whitespace) to not be <?

What Mike is suggesting is in fact exactly the same thing libxml is doing.

Well, with the difference that you guys could probably perform the same check without crashing my program :)

@astrofrog: Yes, if you exclude the doctype declaration (which is technically required, but often omitted in practice), Foo is a completely valid HTML file (the standard says that the <html><body>Foo</body></html> is implied). Also, when considering an HTML fragment that may be preceded by random ignored stuff (which @taldcroft points out we currently support on purpose, and I wasn't aware of), then there is even more likelihood of it not starting with <. In light of all that, I see the sense in @hamogu's suggestion to explicitly look for <tr>.

Doesn't the doctype declaration include < too?

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">

Slightly OT: do you think there's room for a new format hint or two, for slightly more fine-grained control over guessing?

format 1: Basically the current logic: tries HTML, XML, csv, ...
format 2: Tries only csv, fixed width, etc
(maybe) format 3: Tries only DOM-like formats (HTML, XML, VOTable)

@astrofrog - to reiterate what @mdboom said, we support HTML fragments. Think of the use-case of going to a page with one or more tables, click on "View Source", then copy/paste just the table into a local file.

@ChrisBeaumont - that's where I was going with the idea of allowing format to be a list that would limit the guessing to formats within the specified list. Then we could pre-define common lists of formats as needed as a convenience.

Then we could pre-define common lists of formats as needed as a convenience.

I like this idea. Generally I want to put ~0 effort into the keywords passed to my parsing function, and have it "just work". However it's very little extra burden for me to decide and signal whether my file is fixed with, delimited, or dom-like. If that makes the subsequent parsing more reliable, then that sounds like a win.

I don't think you need to look for < as the first character (there are other things than can come before < in an HTML file like a BoM anyways). Just that appears somewhere in the fragment, and go from there. That's what libxml is doing too. In other words, although a document not containing < anywhere can still, in some limited sense, be a valid HTML document that's not what we're looking for. All we care about for this purpose is if it contains an HTML table which it simply can't without finding a <. You could go further and look for specific tags but I might not even bother going that far.

See also comment in #2881 for a way to generalize this idea to all formats to improve guessing performance overall.

PR attached.

-1 on this. It might break people's code who right now rely on guessing of HTML tables. In the mid term fast_identify is a lot more promising and we should aim to do that before 1.1 (at least for HTML tables), so that this code would never be released.

In the short term, I suggest that @ChrisBeaumont just monkey patches the guess list in his use case to look as suggested in this PR. Would that work?

I thought I made this comment somewhere but can't find it - this will not affect people using Table.read since I think that is done based on the extension for HTML?

In any case, as a workaround, we could also make ascii.read behave this way - i.e. disable the guessing as suggested by this PR, but add a special case that if the extension is htm or html, it will actually try the HTML parser and try it first? This will be backward-compatible for 99% of cases.

What happened to the idea of pre-scanning for presence of a < before passing anything to the HTML parser?

@hamogu - to my mind this is a critical bug since it results in segfault and has been reported by at least 5 users. Would you be at least +0 if we did the suggestion from @astrofrog to reduce (to a very low level) the breakage? With his suggestion in place the only thing that would break is if you are passing some HTML table input besides a file name and asking for that to be guessed.

@embray - the problem with pre-scanning is that it still leaves open the loophole of a CSV file that happens to contain valid HTML. If you lower the bar as low as just a < then a false positive is quite likely and the segfault can still appear.

OK, convinced, but we might still want to reverse this once we have improved the guessing with a pretty scanning step.

👍 to @astrofrog's suggestion on this.

I should also note that I've had 2 other people specifically come to me with this problem within the last couple weeks (both are not active on github but it caused them significant problems which they might not have solved w/o me to ask...), so I agree with @taldcroft that this is critical. (Perhaps we should also get this in for 1.0.3, @embray ?)

I'll try to implement the suggestion for io.ascii.read().

OK, maybe we have an acceptable solution to this issue. The default now is to not guess HTML, but if the input is "probably" HTML then the HTML reader is put at the head of the guess list. Please check the code for what is meant by "probably" and let me know if that seems fine.

The idea is to set the bar relatively high for "probably" to avoid false positives with randomly-occurring HTML-like characteristics, but still get most real HTML table inputs for guessing. Nothing is changed in the HTML reader itself.

I think this will result in a small enough change to the behavior in 99% of real use cases that we could put this into a bug fix release, but comments welcome.

Looks good to me (if the tests pass).

Okay, I'll merge for v1.0.3, but note the changelog entry is misplaced, in that case. I will move it after merging.