Fix most of the tokenizer tests. #41
Conversation
|
The documentation is not available anymore as the PR was closed or merged. |
|
Thanks a lot for all this work @lockon-n ! I can have a deeper look on Thursday, is that ok? |
|
Thanks @NielsRogge and @SaulLu! @lockon-n will work with you on this PR. |
| @@ -999,6 +999,34 @@ def post_processor(self): | |||
| ], | |||
| ) | |||
|
|
|||
| class MarkupLMConverter(Converter): | |||
There was a problem hiding this comment.
Do you remember why Roberta's converter didn't work?
There was a problem hiding this comment.
I compare roberta's to bert's, seems it does not add the unknown token to BPE model.
There was a problem hiding this comment.
Thanks! Yes, that's possible as Roberta's is a byte-level BPE (we can't have unknown tokens). MarkupLM's tokenizer can produce unknown tokens?
There was a problem hiding this comment.
Well. We assume MarkupLM's tokenizer is exactly the same as Roberta's, so unknown tokens shouldn't exist. So the only problem is this test is originally designed for LayoutLM, which is based on Bert's tokenizer. Maybe we can ignore it.
There was a problem hiding this comment.
It's test_rust_and_python_full_tokenizers.
You can see in the setUp() of MarkupLMTokenizationTest, we have an unknown token for the temporary slow tokenizer, however if using the default RoBERTa converter, the unknown token is removed when building fast tokenizer. Then ids and rust_ids in line 1122/1123 (test_tokenization_markuplm.py) are not the same (the first token of ids is '<unk>', while rust_ids does not have it).
| overflowing_xpath_tags_seq = pair_xpath_tags_seq[-window_len:] | ||
| overflowing_xpath_subs_seq = pair_xpath_subs_seq[-window_len:] |
| text = [['this', 'is', 'the'], ["how", "are", "you"]] | ||
| xpaths = [["html/body"] * 4, ["html/body"] * 3] |
There was a problem hiding this comment.
I think I'm missing something here: I see 3 nodes in the ['this', 'is', 'the'] list. Why do we have 4 values in the corresponding xpaths list?
There was a problem hiding this comment.
This should be a typo, sorry about that.
There was a problem hiding this comment.
No problem! If it is not possible to have a case where the number of text nodes and xpath nodes is not the same, shouldn't an error be raised (btw I wonder how the tokenizer handles this difference currently)?
There was a problem hiding this comment.
I think raising an error is enough, or something like assume len(text)==len(xpaths). In fact we do not consider such difference when designing relevant module, but xpaths longer than text may be ignored I guess.
| inputs = new_tokenizer(text, xpaths=xpaths) | ||
| self.assertEqual(len(inputs["input_ids"]), 2) | ||
| decoded_input = new_tokenizer.decode(inputs["input_ids"][0], skip_special_tokens=True) | ||
| expected_result = "this is the" | ||
| expected_result = "thisisthe" # original expected result "this is the" seems contradicts to roberta-based tokenizer |
There was a problem hiding this comment.
This behaviour led me at the time to investigate how tokenization was done in the original repository.
For context, in a Roberta tokenizer spaces are transformed into Ġ characters and not removed from the tokenization. With Roberta's tokenizer, the tokenization of the individual words - 'this', 'is', and 'the'- would be:
- with
add_prefix_space=False:['this'],['is'], and['the'] - with
add_prefix_space=True:['Ġthis'],['Ġis'], and['Ġthe']
The spaces being lost and the decoder not artificially adding spaces explains the current behaviour of the decode method. Nevertheless, this led me to 2 questions:
- in the original code did the text in each node start with a space (even if this space was artificily added with a
add_prefix_spaceargument)? In other words, was the tokenization of the nodes[['this'], ['is'], ['the']]or was it[['Ġthis'], ['Ġis'], ['Ġthe']]. - in the case where the text in each node does not start with a space, do we need a custom decoder to reintroduce spaces (is this possible?)? To answer this question I think the most important thing is to know if MarkupLM can solve generative tasks.
These questions led me to propose a new integration test in this PR #37 which is unfortunately based on a very old version of the library and therefore does not make the PR readable now. To create this integration test I investigate the tokenization produced by the authors of MarkUpLM on the downstream task of WebSRC. It seems to me that the integration test I proposed to add was this one:
transformers/tests/test_tokenization_markuplm.py
Lines 2196 to 2251 in 36dcacc
In addition to the problems I had identified in the PR message, the targets defined in the test allowed me to conclude that there was no space added at the beginning of each node (which would answer my first question above). But you would surely know better than I what you did in your original code and what should be our ground truth!
There was a problem hiding this comment.
For question 1, the tokenization should be [['this'], ['is'], ['the']]. For question 2, now MarkupLM can not handle generative questions and I think a custom decoder is not necessary.
Frankly speaking, we do not consider too much about the "prefix space". For each node, we will first do strip() to remove the possible prefix/suffix spaces, and all things left is to use a default Roberta Tokenizer to tokenize the span and generate tokens.
There was a problem hiding this comment.
Very clear to me!
As MarkupLM can't do generative tasks, I think we should put a warning message when the decode method is called saying that the decoding is experimental and subject to change.
There was a problem hiding this comment.
Sure, that's exactly what we want :)
| # Ensure consistency | ||
| output_txt = tokenizer.decode(toks_ids, clean_up_tokenization_spaces=False) | ||
| # an extra blank will cause inconsistency: ["a","b",] & "a b" | ||
| ''' | ||
| if " " not in output_txt and len(toks_ids) > 1: | ||
| output_txt = ( | ||
| tokenizer.decode([toks_ids[0]], clean_up_tokenization_spaces=False) | ||
| + " " | ||
| + tokenizer.decode(toks_ids[1:], clean_up_tokenization_spaces=False) | ||
| ) | ||
| ''' | ||
| if with_prefix_space: | ||
| output_txt = " " + output_txt | ||
| nodes = output_txt.split(" ") | ||
| xpaths = ["html/body" for i in range(len(nodes))] | ||
| output_ids = tokenizer.encode(nodes, xpaths=xpaths, add_special_tokens=False) | ||
|
|
||
| return nodes, xpaths, output_ids |
There was a problem hiding this comment.
For this automatic script, I wonder if what is currently generated is the most useful. Currently nodes and xpaths are both composed of a single element, ['lowerstidner'] and ['html/body'] respectively. Wouldn't it be more useful to have an example here with multiple nodes and xpaths?
There was a problem hiding this comment.
I'm quite confused about this question, would you like to give more details?
There was a problem hiding this comment.
Since in most use cases the input will have several nodes, I think we should also have several nodes in the example returned here. Indeed, by not having several nodes one might not be able to spot potential problems.
For example: ['lowerstidner', 'lower', 'lowersti'] and ['html/body', 'html/body/div', 'html/body/div/table']
There was a problem hiding this comment.
Yes, having multiple nodes is more common and it's better to have an example like that. But I think there's no difference if we only consider the xpath converter part.
I remember that the corresponding test in LayoutLM can generate multiple nodes, which may also be a 'bert2roberta' problem.
There was a problem hiding this comment.
It is possible, I think in this case it is ok to override this method to propose a sufficiently adversarial example. 😊
| @@ -2216,7 +2215,7 @@ def test_markuplm_integration_test(self): | |||
| nodes, xpaths = self.get_nodes_and_xpaths() | |||
|
|
|||
| # fmt: off | |||
| expected_results = {'input_ids': [101, 1037, 6881, 2135, 3231, 102, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0], 'bbox': [[0, 0, 0, 0], [423, 237, 440, 251], [427, 272, 441, 287], [427, 272, 441, 287], [419, 115, 437, 129], [1000, 1000, 1000, 1000], [0, 0, 0, 0], [0, 0, 0, 0], [0, 0, 0, 0], [0, 0, 0, 0], [0, 0, 0, 0], [0, 0, 0, 0], [0, 0, 0, 0], [0, 0, 0, 0], [0, 0, 0, 0], [0, 0, 0, 0], [0, 0, 0, 0], [0, 0, 0, 0], [0, 0, 0, 0], [0, 0, 0, 0]], 'token_type_ids': [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0], 'attention_mask': [1, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]} # noqa: E231 | |||
| expected_results = {'input_ids': [0, 42891, 8331, 2, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1], 'xpath_tags_seq': [[216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216], [109, 25, 50, 120, 50, 178, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216], [109, 25, 50, 120, 50, 178, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216], [216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216], [216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216], [216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216], [216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216], [216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216], [216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216], [216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216], [216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216], [216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216], [216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216], [216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216], [216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216], [216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216], [216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216], [216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216], [216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216], [216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216, 216]], 'xpath_subs_seq': [[1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001], [109, 25, 50, 120, 50, 178, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001], [109, 25, 50, 120, 50, 178, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001], [1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001], [1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001], [1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001], [1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001], [1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001], [1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001], [1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001], [1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001], [1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001], [1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001], [1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001], [1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001], [1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001], [1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001], [1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001], [1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001], [1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001, 1001]], 'token_type_ids': [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0], 'attention_mask': [1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]} | |||
There was a problem hiding this comment.
Thanks for generating the right targets! Out of curiosity, what was your workflow for generating these targets (so that I know how to do it if we want to redo it again for other examples in the futur)?
There was a problem hiding this comment.
As you can see the original expected results are for LayoutLM (e.g., bbox) but not MarkupLM.
To generate it, just use a roberta tokenizer to tokenize it, and do some padding is ok. The xpath tags/subscripts part are from the get_xpath_seq method in MarkupLMTokenizer.
There was a problem hiding this comment.
Thank you very much for your feedback. I wonder if there might be a way to get the targets out of the original code.
The reason I'm wondering this is that while investigating the WebSRC downstream task I noticed that the template (=adding tokens at the beginning of a sentence, at the separation of sentences and at the end of a sentence) the pair of sentences (questions and textual nodes) was not identical to Roberta's. From what I understand:
- Roberta's template:
'<s>', tokens from sentence 1 '</s>', '</s>', tokens from sentence 2 '</s>' - WebSRC MarkupLM's template:
'<s>', tokens from sentence 1 '</s>', tokens from sentence 2 '</s>'
But maybe in the original code several templates were also used depending on the task, or I didn't understand something...
So, in this example I wonder if the target for get_question_nodes_and_xpaths and get_question_nodes_and_xpaths_batch is indeed a target representative of the modeling proposed in MarkupLM.
There was a problem hiding this comment.
Oh I get your point.
The current targets are wrong, and what we want is WebSRC's template, which means the element related to the second </s> should be removed and a padding element should be added in 'input_ids', 'xpath_tags_seq', 'xpath_subs_seq' and 'attention_mask'. We leave 'token_type_ids' alone as it is always all-zero. Sorry for this mistake.
The source code for WebSRC is BERT's style, and the early version for MarkupLM is also BERT's style. In other words, there is only one sep token in between. This feature is kept after we switch to RoBERTa's tokenizer. I have to admit this is our ignorance, because we do not know standard RoBERTa tokenizer inserts two consecutive sep tokens at that time. As a result, our pre-training is also conducted in this manner (one sep), so the template in fine-tuning should also be that kind.
To fix this issue, simply removing the second </s> and its pad in xpath tag/subs is OK in MarkupLMTokenizer.
There was a problem hiding this comment.
Thank you very much for the confirmation! There are really no worries!
Unfortunately this means that for the tokenizer fast we have to wait for a new feature that should arrive soon in the tokenizers library to be able to combine two post-processors: the one for the byte-level and the one for the template that is not the one from roberta.
|
@SaulLu Hi there. I have tried my best to answer the questions. Feel free to raise more lol. |
|
@SaulLu More feedback updated. Looking for your reply ( ,and possibly, more questions). 😄 |
|
Hi @NielsRogge , I think MarkupLM is almost done except that MarkupLMTokenizer will insert two between the question and the page context, so we just need to delete the second one. But the tokenizerfast may wait for some new features as @SaulLu said. |
|
Hi @SaulLu and @NielsRogge, how about the progress of the tokenizer fast? |
|
Hi @NielsRogge and @lockon-n and @wolfshow , As the feature for tokenizers fast is not ready yet, what we can do is to add the fast version of Markup LM with partial availability: the only case that will not work is if I have opened a PR showing this case here: lockon-n#1 I think this is a good compromise to add MarkUp LM to transformers. If we want to use this workaround, the |
proposal of a fix for the MarkupLM fast tokenizer
|
Hi @NielsRogge , I have merged the PR proposed by @SaulLu . I think MakrupLM is ready for transformers now. |
|
Great! 🙌 I just think we need to add an integration test for the fast tokenizer too and change the template for the slow tokenizer with the question 😄 (and add the warning discussed in this message: #41 (comment)) |
…late in slow tokenizer
|
@SaulLu Issues have been fixed in latest update. The integration test for fast tokenzier fails (double problem) when text-pair encoding, is that expected? I have set trim_offset as False in default. |
| def test_markuplm_integration_test(self): | ||
|
|
||
| tokenizer_p = MarkupLMTokenizer.from_pretrained("microsoft/markuplm-base") | ||
| # no fast tokenizer for now | ||
| # tokenizer_r = MarkupLMTokenizerFast.from_pretrained("microsoft/markuplm-base") | ||
| tokenizer_r = MarkupLMTokenizerFast.from_pretrained("microsoft/markuplm-base") |
There was a problem hiding this comment.
@lockon-n , can you re-try with this change? 🙏
| tokenizer_r = MarkupLMTokenizerFast.from_pretrained("microsoft/markuplm-base") | |
| tokenizer_r = MarkupLMTokenizerFast.from_pretrained("microsoft/markuplm-base", from_slow=True) |
If it solves the issue it's just because the ŧokenizer.json file at https://huggingface.co/microsoft/markuplm-base needs to be updated
There was a problem hiding this comment.
Oh 😞
Could this be coming from the xpath_tags_seq list?
I have trouble remembering how the nodes and xpaths need to be tied. I particular I don't remember why line 102 is xpaths = ["/html/body/div/li[1]/div/span", "/html/body/div/li[1]/div/span"] and not xpaths = ["/html/body/div/li[1]/div/span"]
transformers/tests/models/markuplm/test_tokenization_markuplm.py
Lines 99 to 112 in 485b2ca
There was a problem hiding this comment.
This seems to be a typo, but does not leads to mistakes. The second xpath is discarded when processing I guess.
|
|
||
| tokenizer.post_processor = processors.TemplateProcessing( | ||
| single="<s> $A </s>", | ||
| pair="<s> $A </s> $B </s>", | ||
| special_tokens=[ | ||
| ("<s>", ot.convert_tokens_to_ids("<s>")), | ||
| ("</s>", ot.convert_tokens_to_ids("</s>")), | ||
| ], | ||
| ) |
There was a problem hiding this comment.
oh I'm sorry the change on this file slipped through the cracks when I commited the files.... but indeed this is the fix!
There was a problem hiding this comment.
@SaulLu I rewrite the converter, this solve the double problem. i.e., the tokenizer.post_processor(). However, the tokenizerfast seem to tokenize "what's his name?" as ["what","'s","his","name","?"] while the slow-tokenizer and groundtruth is ["what","'s","Ġhis","Ġname","?"]. Seems the space problem caused by trim_offsets?
There was a problem hiding this comment.
@SaulLu I rewrite the converter, this solve the double problem. i.e., the tokenizer.post_processor().
🙌
However, the tokenizerfast seem to tokenize "what's his name?" as ["what","'s","his","name","?"] while as ["what","'s","Ġhis","Ġname","?"]. Seems the space problem caused by trim_offsets?
Can you share the snippet you used to see that please?
There was a problem hiding this comment.
run test_markuplm_integration_test(), and you can compare the difference between encoding_p['input_ids] and encoding_r['input_ids'] in Line 2291&2292. I convert the ids into tokens, and gets the results above.
Sorry for not using an easier way like a snippet of codes.
There was a problem hiding this comment.
So... we reach a release-ready version?
There was a problem hiding this comment.
Thanks so much for your effort !!!! 👏 👏 really nice experience to collaborate with such nice staffs, also excited to see our model appearing in this famous repo (a common dream for NLPers I guess 😆)
| tokenizer.post_processor = processors.TemplateProcessing( | ||
| single="<s> $A </s>", | ||
| pair="<s> $A </s> $B </s>", | ||
| special_tokens=[ | ||
| ("<s>", ot.convert_tokens_to_ids("<s>")), | ||
| ("</s>", ot.convert_tokens_to_ids("</s>")), | ||
| ], | ||
| ) |
There was a problem hiding this comment.
| tokenizer.post_processor = processors.TemplateProcessing( | |
| single="<s> $A </s>", | |
| pair="<s> $A </s> $B </s>", | |
| special_tokens=[ | |
| ("<s>", ot.convert_tokens_to_ids("<s>")), | |
| ("</s>", ot.convert_tokens_to_ids("</s>")), | |
| ], | |
| ) | |
| cls = str(self.original_tokenizer.cls_token) | |
| sep = str(self.original_tokenizer.sep_token) | |
| cls_token_id = self.original_tokenizer.cls_token_id | |
| sep_token_id = self.original_tokenizer.sep_token_id | |
| tokenizer.post_processor = processors.TemplateProcessing( | |
| single="cls $A sep", | |
| pair="cls $A sep $B sep", | |
| special_tokens=[ | |
| (cls, cls_token_id), | |
| (sep, sep_token_id), | |
| ], | |
| ) |
|
Ok, awesome work :) can you confirm I can merge this now? |
|
I think yes @NielsRogge |
yes! 🙌 |
|
So I've merged + rebased with the main branch of Transformers, seems like there are 4 tests to be fixed. Running returns: |
|
@NielsRogge Hi, I have modify the codes to tackle these tests. Should another PR be raised? |
|
Yes, you can do that. Note that I've rebased the |
What does this PR do?
I have fixed most of the tokenizer tests except
test_maximum_encoding_length_pair_input.Seems these tests are copied from LayoutLMv2, but LayoutLMv2 is based on BERT tokenizer, so some tests may not suitable for MarkupLM which is based on RoBERTa tokenizer. I tried my best to fix both the test part and the tokenizer part, with necessary annotations left. However,
test_maximum_encoding_length_pair_inputis beyond my ability, I only fixed the slow tokenizer part in this test.I will be very pleased if this commit is useful!
Fixes # (issue)
Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
@NielsRogge Feel free to ask questions or give suggestions for changes.