RES-2190: Fix labels set to -100 in finetuning tasks #517

benja-matic · 2021-05-18T20:18:53Z

Regarding RES-2190. Looks like HuggingFace trainer looks at args.dataloader_drop_last for train, and eval loaders. Workaround is to turn flip that to False during evaluation in compute_metrics_task, flip it back to True at the end (if it was originally true). A few minor ignorable formatting edits will merge in as well. Finally, there's a finetuning experiment for tiny_bert50k.

…uation samples

mvacaporale · 2021-05-19T23:53:28Z

Good find. The solution is simple and straightforward. Note, there could be others ways. For one, we could override get_eval_dataloader, but I think that would a bit cumbersome. I think yours will work quite well for our needs.

mvacaporale · 2021-05-19T21:20:36Z

projects/transformers/experiments/finetuning.py

@@ -144,6 +144,12 @@
 )


+initial_finetuning_tiny_bert50k_glue = deepcopy(finetuning_bert700k_glue)
+initial_finetuning_tiny_bert50k_glue.update(


Why initial_finetuning_tiny_bert50k_glue as opposed to just finetuning_tiny_bert50k_glue? What does the initial signify?

mvacaporale · 2021-05-19T21:23:15Z

projects/transformers/run.py

@@ -52,7 +52,7 @@
 from transformers.integrations import is_wandb_available
 from transformers.trainer_utils import get_last_checkpoint, is_main_process

-from experiments import CONFIGS
+from experiments import CONFIGS 


I think you added an extra space at the end of CONFIGS. Make sure you run flake8 in whatever repo you're submitting a PR for to catch these issues.

cd ~/nta/nupic.research flake8

mvacaporale · 2021-05-19T21:24:13Z

projects/transformers/run.py

    test_dataset = None
    if ((data_args.task_name is not None or data_args.test_file is not None)
-       and training_args.do_predict):
+    and training_args.do_predict):


You removed some whitespace here. Flake8 would have caught this as well.

mvacaporale · 2021-05-19T23:38:23Z

projects/transformers/run_utils.py

+        vals, cnts = np.unique(np.array(eval_dataset['label']), return_counts=True)
+        logging.info(f"Label distribution for {task} before calling trainer.evaluate")
+        for val in range(len(vals)):
+            logging.info(f"Label={vals[val]}: {cnts[val]} examples")


Did you intentionally mean to keep this logging? Do you think we need it?

mvacaporale · 2021-05-19T23:44:14Z

projects/transformers/run_utils.py

-    preds = preds[np.where(ep.label_ids != -100)]
-    logging.info(f"Removing {1-(len(label_ids) / len(ep.label_ids)):.2%} samples "
-                 "from evaluation set where label == -100")
+    assert -100 not in ep.label_ids, "unknown source of -100 labels"


Nit pick: Could you fix the punctuation? "Unknown source of -100 labels."

mvacaporale · 2021-05-19T23:45:46Z

projects/transformers/run_utils.py

@@ -916,7 +930,7 @@ def model_init():
            "HP search with saved models not supported."
        logging.info("Pretraining new model from scratch")

-        # Instantiate model; possibly one of our custom sparse models.
+    # Instantiate model; possibly one of our custom sparse models.


The indentation looks like it was changed here as well.

lucasosouza

Also seems like a good solution to me, it is a hack but it should work fine and with no extra computation or memory required. I left some very minor comments, please take a look

lucasosouza · 2021-05-20T04:20:39Z

projects/transformers/run.py

-        test_dataset = tokenized_datasets[
-            "test_matched" if data_args.task_name == "mnli" else "test"
-        ]
+    if (data_args.task_name is not None or data_args.test_file is not None):


I didn't understand this change, what is the difference between the old version and the new?

No semantic difference. I just couldn't figure out how to make it pep compliant. I was getting issues about the indent, or the length of the line, so just broke it up.

lucasosouza · 2021-05-20T04:27:03Z

projects/transformers/run_utils.py

+    # if you want drop_last for training, this toggles it back on
+    if drop_last:
+        trainer.args.dataloader_drop_last = True
+        logging.info("Switched trainer.args.dataloader_drop_last"


I think we should lower the level of this logging, logging.debug should be enough. There is no useful information to the user here, it is more of a print for debugging purposes.

Got it. Those were mainly to verify that my fix was working properly, not needed going forward. I removed the bit above and a couple related ones.

mvacaporale · 2021-05-20T18:18:51Z

@benja-matic Do you know how this change affects the fine-tuning results? We should probably rerun those from the README (including bert_1mi, bert_100k, sparse_80%_kd_onecycle_lr_rigl, and sparse_80%_kd_onecycle_lr) @lucasosouza What do you think? Is this necessary? My concern is that it may make it harder to contextualize new results if we don't rerun previous fine-tuning experiments. Or at the very least, we should rerun one or two just to make sure the results are negligibly affected.

benja-matic · 2021-05-20T19:56:19Z

@benja-matic Do you know how this change affects the fine-tuning results? We should probably rerun those from the README (including bert_1mi, bert_100k, sparse_80%_kd_onecycle_lr_rigl, and sparse_80%_kd_onecycle_lr) @lucasosouza What do you think? Is this necessary? My concern is that it may make it harder to contextualize new results if we don't rerun previous fine-tuning experiments. Or at the very least, we should rerun one or two just to make sure the results are negligibly affected.

@mvacaporale don't know just yet. I'm rerunning fine tuning on baseline models this afternoon.

… called

Ben Cohen added 2 commits May 18, 2021 15:47

Changer HF trainer.args as you go to avoid drop_last leaving off eval…

b55af86

…uation samples

remove comments

685c30e

benja-matic requested review from lucasosouza and mvacaporale May 18, 2021 20:18

benja-matic added 4 commits May 19, 2021 19:35

Merge branch 'master' into master

edf538b

Merge branch 'master' of github.com:numenta/nupic.research

19d3034

small comment, see if CI will accept current user.email

10171b6

Merge branch 'master' of github.com:benja-matic/nupic.research

f172704

mvacaporale approved these changes May 19, 2021

View reviewed changes

flake 8 fixes, adjustments based on Michaelangelo suggestions

ecfca85

lucasosouza requested changes May 20, 2021

View reviewed changes

lucasosouza reviewed May 20, 2021

View reviewed changes

lucasosouza approved these changes May 20, 2021

View reviewed changes

less verbose logging

41f2d3a

benja-matic added 3 commits May 24, 2021 13:50

Merge branch 'master' into master

bf51acf

added a fix for every time trainer.evaluate or trainer.predictions is…

d7a605a

… called

fixed style

101a097

benja-matic merged commit bc38d37 into numenta:master May 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RES-2190: Fix labels set to -100 in finetuning tasks #517

RES-2190: Fix labels set to -100 in finetuning tasks #517

benja-matic commented May 18, 2021

mvacaporale commented May 19, 2021 •

edited

Loading

mvacaporale May 19, 2021

mvacaporale May 19, 2021

mvacaporale May 19, 2021

mvacaporale May 19, 2021

mvacaporale May 19, 2021

mvacaporale May 19, 2021

lucasosouza left a comment

lucasosouza May 20, 2021

benja-matic May 20, 2021

lucasosouza May 20, 2021

benja-matic May 20, 2021

mvacaporale commented May 20, 2021 •

edited

Loading

benja-matic commented May 20, 2021

RES-2190: Fix labels set to -100 in finetuning tasks #517

RES-2190: Fix labels set to -100 in finetuning tasks #517

Conversation

benja-matic commented May 18, 2021

mvacaporale commented May 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasosouza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvacaporale commented May 20, 2021 • edited Loading

benja-matic commented May 20, 2021

mvacaporale commented May 19, 2021 •

edited

Loading

mvacaporale commented May 20, 2021 •

edited

Loading