New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add LayoutLMv2ForRelationExtraction #19120

Closed

NielsRogge wants to merge 9 commits into huggingface:main from NielsRogge:add_layoutlm_relation_extraction

Contributor

NielsRogge commented Sep 20, 2022

What does this PR do?

This PR adds the relation extraction head of LayoutLMv2, which was a highly requested feature as seen in #14330 #15451 #18091

Niels Rogge and others added 9 commits

September 18, 2022 19:09


          Add first draft

9fb42f3


          Fix bug

bfa9fbe


          Fix output

36c41a1


          Add return_dict option

0c05911


          Add test

219a4f7


          Make most tests pass

4c5c3dc


          Make more tests pass

cf7e2a8


          Improve docstrign

3a904ea


          Make all tests pass

1e844fa

NielsRogge requested a review from sgugger

September 20, 2022 09:19

HuggingFaceDocBuilderDev commented Sep 20, 2022

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

Contributor Author

NielsRogge commented Sep 20, 2022 •

edited

Loading

@sgugger I'm getting the following error from make fixup:

Checking all objects are properly documented.
Traceback (most recent call last):
  File "/home/niels/python_projects/transformers/utils/check_repo.py", line 788, in <module>
    check_repo_quality()
  File "/home/niels/python_projects/transformers/utils/check_repo.py", line 782, in check_repo_quality
    check_all_objects_are_documented()
  File "/home/niels/python_projects/transformers/utils/check_repo.py", line 693, in check_all_objects_are_documented
    raise Exception(
Exception: The following objects are in the public init so should be documented:
 - LayoutLMv2ForRelationExtraction

However, this model is added to layoutlmv2.mdx, so not sure why this error occurs.

sgugger reviewed

View reviewed changes

Collaborator

sgugger left a comment

Thanks for the PR, but really not convinced by the model added as it is. Some preprocessing code is part of the model, which is not how we usually do things in Transformers. Some inputs are not tensors, which causes multiple problems (tests overridden are just the tip of the iceberg) as a result.

The whole preprocessing should be added as a method on the preprocessing class of LayoutLMv2 and the model should accept tensors, listed as keyword arguments and not inside a dictionary.

src/transformers/models/layoutlmv2/modeling_layoutlmv2.py

+                  Class for outputs of [`LayoutLMv2ForRelationExtraction`].
+                  Args:
+                      loss (`torch.FloatTensor` of shape `(1,)`:

Collaborator

sgugger Sep 20, 2022

The result of PyTorch loss functions are 0d tensors, so this is not accurate.

src/transformers/models/layoutlmv2/modeling_layoutlmv2.py

+                  Args:
+                      loss (`torch.FloatTensor` of shape `(1,)`:
+                          Classification (or regression if config.num_labels==1) loss.

Collaborator

sgugger Sep 20, 2022

Reading the code, it's always a classification loss, so this needs to be adapted.

src/transformers/models/layoutlmv2/modeling_layoutlmv2.py

Comment on lines +1481 to +1486

+                      - x_1: `(N, *, in_features)` where `N` is the batch dimension and `*` means any number of additional
+                        dimensisons.
+                      - x_2: `(N, *, in_features)`, where `N` is the batch dimension and `*` means any number of additional
+                        dimensions.
+                      - Output: `(N, *, out_features)`, where `N` is the batch dimension and `*` means any number
+                          of additional dimensions.

Collaborator

sgugger Sep 20, 2022

No one-letter variables please. Especially not in the documentation which we don't mind being long. Here just say (batch_size, *, features) where * means any number of additional dimensisons.

src/transformers/models/layoutlmv2/modeling_layoutlmv2.py

+                  """
+                  def __init__(self, in_features, out_features):
+                      super(BiaffineAttention, self).__init__()

Collaborator

sgugger Sep 20, 2022

Suggested change

      
                    super(BiaffineAttention, self).__init__()
          
                    super().__init__()

Python 2 was dead two years ago...

src/transformers/models/layoutlmv2/modeling_layoutlmv2.py

+                      self.ffnn_head = copy.deepcopy(projection)
+                      self.ffnn_tail = copy.deepcopy(projection)
+                      self.rel_classifier = BiaffineAttention(config.hidden_size // 2, 2)
+                      self.loss_fct = CrossEntropyLoss()

Collaborator

sgugger Sep 20, 2022

No need to hard-code this here. Just use it when necessary in the forward.

src/transformers/models/layoutlmv2/modeling_layoutlmv2.py

Comment on lines +1586 to +1589

+                          head_repr = torch.cat(
+                              (hidden_states[b][head_index], head_label_repr),
+                              dim=-1,
+                          )

Collaborator

sgugger Sep 20, 2022

Suggested change

      
                        head_repr = torch.cat(
          
                            (hidden_states[b][head_index], head_label_repr),
          
                            dim=-1,
          
                        )
          
                        head_repr = torch.cat((hidden_states[b][head_index], head_label_repr), dim=-1)

Fits in one line.

src/transformers/models/layoutlmv2/modeling_layoutlmv2.py

Comment on lines +1590 to +1593

+                          tail_repr = torch.cat(
+                              (hidden_states[b][tail_index], tail_label_repr),
+                              dim=-1,
+                          )

Collaborator

sgugger Sep 20, 2022

Suggested change

      
                        tail_repr = torch.cat(
          
                            (hidden_states[b][tail_index], tail_label_repr),
          
                            dim=-1,
          
                        )
          
                        taill_repr = torch.cat((hidden_states[b][tail_index], tail_label_repr), dim=-1)

src/transformers/models/layoutlmv2/modeling_layoutlmv2.py

+                      self.rel_classifier = BiaffineAttention(config.hidden_size // 2, 2)
+                      self.loss_fct = CrossEntropyLoss()
+                  def build_relation(self, relations, entities):

Collaborator

sgugger Sep 20, 2022

This looks like a preprocessing method. It shouldn't be part of the model.

src/transformers/models/layoutlmv2/modeling_layoutlmv2.py

Comment on lines +1665 to +1666

		>>> encoding["entities"] = [{"start": [0, 4], "end": [3, 6], "label": [2, 1]}]
		>>> encoding["relations"] = [{"start_index": [], "end_index": [], "head": [], "tail": []}]

Collaborator

sgugger Sep 20, 2022

Model cant take lists as inputs as they then won't work with ONNX/distributed etc. This should all be tensors.

tests/models/layoutlmv2/test_modeling_layoutlmv2.py

+                          # (Even with this call, there are still memory leak by ~0.04MB)
+                          self.clear_torch_jit_class_registry()
+                  # overwrite as LayoutLMv2ForRelationExtraction outputs dictonaries containing integers rather than tensors

Collaborator

sgugger Sep 20, 2022

Which really shouldn't be the case...

NielsRogge mentioned this pull request

[WIP] Implement LayoutLMv2ForRelationExtraction (continues #15173) #18943

Closed

github-actions bot commented Oct 20, 2022

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

github-actions bot closed this

lamaeldo commented Apr 10, 2023

Hey @NielsRogge , any chance that this will ever be implemented?
Looking around the history of the PR and Iissues, it seems that there was a fair bit of interest

Contributor Author

NielsRogge commented Apr 10, 2023

The reason the PR wasn't merged is because models need to output fixed size tensors, to make sure things like distributed training and ONNX export work. However LayoutLMv2ForRelationExtraction outputs lists of tensors in its current implementation, due to each example in the batch having a different amount of relations. So we would need to pad them up to a fixed size such that the model outputs fixed size tensors.

Haven't looked into that yet but if you're willing to contribute, let me know!

Btw I do have a notebook on fine-tuning this model here.

amyeroberts mentioned this pull request

'loss': 0.0, 'grad_norm': nan, 'learning_rate': 0.0, #32382

Closed

4 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet