-
Notifications
You must be signed in to change notification settings - Fork 885
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.
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
multilingual coreference and singletons support #1403
Conversation
stanza/models/mwt/trainer.py
Outdated
@@ -87,10 +87,6 @@ def predict(self, batch, unsort=True): | |||
pred_tokens.append("".join(pred_seq)) | |||
else: | |||
pred_tokens = ["".join(seq) for seq in pred_seqs] # join chars to be tokens | |||
# if any tokens are predicted to expand to blank, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly forgot why this is removed; perhaps it is a merging artifact?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect so. I just made a couple changes on the MWT processing last week in order to fix some weird tokenization of previously unknown lemmas in Spanish
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, would try to merge in that change or otherwise undo this. shouldn't be here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
@staticmethod | ||
def _get_pair_matrix(all_mentions: torch.Tensor, | ||
mentions_batch: torch.Tensor, | ||
def _get_pair_matrix(mentions_batch: torch.Tensor, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the change here is to push the dereferencing up into the caller? sounds fair, could maybe split that out for readability of the PR but it's not necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also the indexing is required multiple times instead of one, so I felt it would be easier than passing everything around to two stacks.
happy to split it, do you mean I should undo the change and apply another diff?
thanks in advance!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no change needed, was just thinking in terms of making the big change more readable with smaller pieces cut off. it's not an issue though.
you can if you like split off individual edits with git rebase -i dev
and then edit
the change. it's really not necessary in this case, though, unless you wanted the practice
stanza/models/coref/bert.py
Outdated
@@ -40,14 +44,19 @@ def get_subwords_batches(doc: Doc, | |||
while end and doc["sent_id"][doc["word_id"][end - 1]] == sent_id: | |||
end -= 1 | |||
|
|||
# if we ended up at prev end, well, looks like we will |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is what happens if a single sentence is longer than the maximum length of the transformer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, clarified comment
@@ -15,7 +21,24 @@ def __init__(self): | |||
self._r = 0.0 | |||
self._p_weight = 0.0 | |||
self._r_weight = 0.0 | |||
self._num_preds = 0.0 | |||
|
|||
# muc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these names are somewhat opaque but i assume they're just the standard scoring names? seems reasonable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated with better variable names; they are underscore prefixed, so hopefully folks won't try to access it from the outside
stanza/models/coref/conll.py
Outdated
try: | ||
return int(split[-1].replace(")", "").strip()) | ||
except ValueError: | ||
breakpoint() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
stanza/models/coref/conll.py
Outdated
cluster_info_lst.append(f"e{cluster_marker})") | ||
|
||
|
||
# we need our clusters to be ordered such that the one that closest first is listed last |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that closest first
-->
that is closest to the first
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
stanza/models/coref/conll.py
Outdated
breakpoint() | ||
else: | ||
# we want everything that's a closer to be first | ||
return 1000000000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about float('inf')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
of course; no clue why I did this....
#dev_data = "data/coref/corefud_concat_v1_0_langid-bal.dev.json" | ||
#test_data = "data/coref/corefud_concat_v1_0_langid-bal.test.json" | ||
|
||
train_data = "data/coref/corefud_concat_v1_0_langid.train.json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a script or an explanation of how to build this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering if some of the others could be cleaned up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. left the ontonotes + gum + balanced langid. Also, looks like I missed the conversion script being committed: would love your review.
convert_udcoref.py
: converts depparse annotated udcoref files into our formatbalance_languages.py
: takes a dataset built in the previous option and balance the document counts for each language within the JSON.
lora_dropout = 0.1 | ||
lora_alpha = 128 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be undone just to make the change cleaner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. no clue how this ended up changed. apologies
@@ -58,7 +60,18 @@ def forward(self, # type: ignore # pylint: disable=arguments-differ #35566 in | |||
distance = torch.where(distance < 5, distance - 1, log_distance + 2) | |||
distance = self.distance_emb(distance) | |||
|
|||
genre = torch.tensor(self.genre2int[doc["document_id"][:2]], | |||
if not self.__full_pw: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is for VRAM OOM issues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not quite: this is for documents that have a genre and speaker embedding, which doesn't exist for UDCoref
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, got it. is that detected automatically from the input files? i might have missed that if it is. if not, it would be simpler for the user to do that rather than make it an option
stanza/models/coref/rough_scorer.py
Outdated
@@ -29,6 +29,7 @@ def forward(self, # type: ignore # pylint: disable=arguments-differ #35566 in | |||
Returns rough anaphoricity scores for candidates, which consist of | |||
the bilinear output of the current model summed with mention scores. | |||
""" | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe undo just to keep things cleaner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
help="Adjust the dummy mix") | ||
argparser.add_argument("--bert_finetune_begin_epoch", type=float, | ||
help="Adjust the bert finetune begin epoch") | ||
argparser.add_argument("--warm_start", action="store_true", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
worth adding an argument for --full_pairwise
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. I expect that for non ontonotes documents this will be rarely used, however, because most datasets don't have speaker embeddings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heh, adding that flag might have been the opposite of what i just suggested above with the __full_pw
option
stanza/models/coref/model.py
Outdated
@@ -30,13 +30,67 @@ | |||
from stanza.models.coref.utils import GraphNode | |||
from stanza.models.coref.word_encoder import WordEncoder | |||
|
|||
from torch.utils.data import Dataset | |||
from functools import lru_cache, wraps | |||
import weakref |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this still used? maybe it could go away
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, apologies. used to be used for dataset memoization, but turns out we were system OOMing on long docs instead
stanza/models/coref/model.py
Outdated
@@ -30,13 +30,67 @@ | |||
from stanza.models.coref.utils import GraphNode | |||
from stanza.models.coref.word_encoder import WordEncoder | |||
|
|||
from torch.utils.data import Dataset | |||
from functools import lru_cache, wraps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same with wraps
, a quick ctrl-f doesn't find it anywhere else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed, thanks
stanza/models/coref/model.py
Outdated
from peft import LoraConfig, get_peft_model, get_peft_model_state_dict, set_peft_model_state_dict | ||
|
||
from stanza.utils.get_tqdm import get_tqdm # type: ignore | ||
tqdm = get_tqdm() | ||
|
||
logger = logging.getLogger('stanza') | ||
|
||
class CorefDataset(Dataset): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can / should this be refactored into a different module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
stanza/models/coref/model.py
Outdated
self.config = config | ||
self.tokenizer = tokenizer | ||
|
||
self.__filter_func = TOKENIZER_FILTERS.get(self.config.bert_model, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe leave a comment here to specify that the default is to not filter anything? it takes a couple seconds to understand, so maybe that time can be saved for the reader instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
stanza/models/coref/model.py
Outdated
@@ -91,14 +144,15 @@ def __init__(self, | |||
modules_to_save=self.config.lora_fully_tune, | |||
bias="none") | |||
|
|||
self.bert = get_peft_model(self.bert, self.__peft_config) | |||
self.bert.train() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this switch necessary? it was working before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted. There's a chance that I did this because certain types of loading loads in eval, which makes PEFT do weird things. But, I can't seem to reproduce it. Apologies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no worries, mostly was wondering about needing to change other models
self.bert.train() | ||
self.bert = get_peft_model(self.bert, self.__peft_config) | ||
self.trainable["bert"] = self.bert | ||
|
||
if build_optimizers: | ||
self._build_optimizers() | ||
self._set_training(False) | ||
self._coref_criterion = CorefLoss(self.config.bce_loss_weight) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe a comment on the distinction between the coref & rough criterions would be helpful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
stanza/models/coref/model.py
Outdated
@@ -117,13 +171,15 @@ def training(self, new_value: bool): | |||
@torch.no_grad() | |||
def evaluate(self, | |||
data_split: str = "dev", | |||
word_level_conll: bool = False | |||
word_level_conll: bool = False, | |||
eval_lang=None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consistency on the typing might be nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
stanza/models/coref/model.py
Outdated
@@ -185,8 +244,9 @@ def evaluate(self, | |||
f" p: {s_lea[1]:.5f}," | |||
f" r: {s_lea[2]:<.5f}" | |||
) | |||
logger.info(f"BAKE!: {w_checker.bakeoff:.5f}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i do think a more informative log line would be helpful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. apologies
@@ -421,12 +488,17 @@ def train(self, log=False): | |||
for doc_indx, doc_id in enumerate(pbar): | |||
doc = docs[doc_id] | |||
|
|||
# skip very long documents during training time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could this be an option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we discussed this being strictly good—simply because it quadruples the memory limit during training and seems to confer no actual performance benefits. happy to make this a flag if needed, however.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's fair. although i was thinking that there could be a cutoff where those lines become batches by themselves. however, it's also not necessary to do that, i think, especially if it's not giving any benefit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe a comment on how many training lines will be skipped, in that case?
running_c_loss += c_loss.item() | ||
running_s_loss += s_loss.item() | ||
|
||
# log every 50 docs | ||
if log and doc_indx % 50 == 0: | ||
# log every 100 docs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could also be an option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
happy to do so; do you think there are other areas where this flag would be used? i.e.: this only affects wandb logs; would love to hear how best I could implement this.
Thanks in advance!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eh, guess not. having some default behavior for the logs is fine until someone complains about wanting more granularity
@@ -490,30 +564,44 @@ def train(self, log=False): | |||
# ========================================================= Private methods | |||
|
|||
def _bertify(self, doc: Doc) -> torch.Tensor: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some similar pieces of logic are also in models/common/bert_embedding.py
not saying this needs to happen this time around, but it would be useful to unify that so there's just one source of truth
that code also handles some other model types, such as the VI extension to bert (phobert)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like the original design put bert.py
there as bert utilities (which has no logic dependent upon bert choice/initialization), whereas this does. happy to refact if you think that's best
stanza/models/coref/model.py
Outdated
y = (y == cluster_ids.unsqueeze(1)) # True if coreferent | ||
# For all rows with no gold antecedents setting dummy to True | ||
y[y.sum(dim=1) == 0, 0] = True | ||
|
||
if singletons: | ||
# add another dummy for firts coref |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops; thanks
Overall it looks good, thanks! Just a bunch of random nitpicks and of course the MWT code being reverted. I would think that with the config changes the way they are, the original model is no longer viable, right? Or does loading the pipeline with the old model and the new code still work? If the old model is dead (which is fine) we should either fix the existing .pt file or rebuild it. |
658f843
to
9d941f0
Compare
IS_UDCOREF_FORMAT = True | ||
UDCOREF_ADDN = 0 if not IS_UDCOREF_FORMAT else 1 | ||
|
||
# TODO: move this to a utility module and try it on other languages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is just a copy of the one in convert_ontonotes.py
, right? can we refactor that now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, yes, absolutely. will do ASAP with the other items today (just got a temp laptop, setting up dev env)
closing in favor of #1406 |
adds support for multilingual and singletons support through
xlm-roberta-large
andt5-large
.