Add NB for BERT models #114

hkirvesl · 2022-09-29T16:41:23Z

Reference issues/PRs

Fixes #81 By adding a notebook on BERT models with topological interpretability methods built on top of one. The old tutorials on Q&A and Translation remain for now.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Description

Add notebook for training HuggingFace Bert model with giotto-deep, and how to use topological pruning methods for such models.
Add support for training HuggingFace models with an unwrapper in Trainer.py
Modify models/utils.py to support the the HuggingFace models (now with an unwrapper)

Screenshots (if appropriate)

Any other comments?

Checklist

I have read the guidelines for contributing.
My code follows the code style of this project. I used flake8 to check my Python changes.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed. I used pytest to check this on Python tests.

review-notebook-app · 2022-09-29T16:41:27Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

matteocao

Once the CI pass, I will study your notebook in details, as I am really curious about it!

matteocao · 2022-09-29T16:51:51Z

examples/basic_tutorial_BERT.ipynb

+  "kernelspec": {
+   "display_name": "Python [conda env:torch] *",
+   "language": "python",
+   "name": "conda-env-torch-py"


the CI is complaining about this name:

jupyter_client.kernelspec.NoSuchKernel: No such kernel named conda-env-torch-py

Can you please change this to:

"kernelspec": { "display_name": "Python 3 (ipykernel)", "language": "python", "name": "python3" },

Thank you! Should be fixed now

@hkirvesl can you please use os.path.join to build file path please? windows and posix OS have a different convention (back and forward slashes)

Thank you for the suggestion. Done.

…hkdev pull the test fixes

there was a mismatch in where data was downloaded and read from, causing the CI to fail.

matteocao

Dear @hkirvesl ,

I think we are almost there: I only have a few minor comments.
I will then review the notebbook in detail.

One important note: did you pull master to your fork before committing? I ask because there is one item in the CI that took > 5h to run and has currently not yet finished (though it should really take 25 mins at most) and I wonder if it is related to not having integrated the fixes. If you have done so, than ignore this last comment

matteocao · 2022-09-30T11:57:28Z

.pre-commit-config.yaml

+#-   repo: https://github.com/psf/black
+#    rev: 22.8.0
+#    hooks:
+#    -   id: black


I understood from raphael that mypy is causing issues, however black should not: it should simply frmat the code properly: why are you commenting black out?

Yes, please keep the hook for black.

Could you please wait until my pull request is accepted and then pull again from master? I fixed all mypy errors, and the pre-commit does work now.

Thanks for the comments, Matteo and Raphael. I did recall some discussion about problems with the prehooks and commenting them out being the recommended temporary work around. I just forgot the details and ended up commenting out too much. All these should be fixed now. Thank you Raphael for the fix, this makes our lives much easier.

matteocao · 2022-09-30T11:59:56Z

gdeep/models/utils.py

@@ -70,6 +70,6 @@ def __call__(
                the output tensor of the module
        """
        if isinstance(module_out, tuple):
-            self.outputs.append(module_out[0].detach())
+            self.outputs.append(module_out[0])#.detach())


These functionalities are needed in the forward hooks in order to extract the activation values of the layers for example.
Could you please double check if the functionalities to get activations values still works properly without the detachment? Put differently, do you understand the consequences of removing the .detach() for the forward hooks? If so, could you please write a couple of sentences here below? thanks!

Thanks for the comment, this is a very good point indeed. I do understand the consequences and I agree what was proposed is not a good solution. To be honest, I forgot about this myself. It was just a quick and dirty fix to make the BERT models work and since it worked and seemingly didn't break any tests I just proceeded without worrying about it.

A good solution would keep the old functionality where applicable, in the same spirit as the change in trainer.py. I believe this has now been fixed.

matteocao · 2022-09-30T12:00:09Z

gdeep/trainer/trainer.py

@@ -266,9 +266,13 @@ def _send_to_device(
                new_x.append(xi.to(DEVICE))
            x = new_x
            prediction = self.model(*x)
+            if(hasattr(prediction, "logits")): # unwrapper for HuggingFace BERT model
+                prediction = prediction.logits # unwrapper for HuggingFace BERT model


all good here!

matteocao · 2022-09-30T13:29:19Z

examples/basic_tutorial_BERT.ipynb

@@ -0,0 +1,1198 @@
+{


here you can simply use
from gdeep.utility import DEVICE

Reply via ReviewNB

Thanks! It is a more giotto-way to do this. In general, any other comments on how to make the notebook more giotto-like are highly appreciated.

matteocao · 2022-09-30T13:29:19Z

examples/basic_tutorial_BERT.ipynb

@@ -0,0 +1,1198 @@
+{


ca you please preceed this cell with a markdown cell describing what you are doing here?

if this cell and the above one are related to handling masks, then maybe it is worth putting a bit of explanation 2 cells above, details the steps that one needs to take

Reply via ReviewNB

Good idea.I added a brief note (and deleted an unnecessary chunk)

matteocao · 2022-09-30T13:29:19Z

examples/basic_tutorial_BERT.ipynb

@@ -0,0 +1,1198 @@
+{


can you please add a few inline comments to describe what you are doing here?

Reply via ReviewNB

matteocao · 2022-09-30T13:29:19Z

examples/basic_tutorial_BERT.ipynb

@@ -0,0 +1,1198 @@
+{


the comment here , I think, needs to be corrected: raw -> mean aggregated

Reply via ReviewNB

You are absolutely right. Fixed.

matteocao · 2022-09-30T13:29:20Z

examples/basic_tutorial_BERT.ipynb

@@ -0,0 +1,1198 @@
+{


some more comments here please

Reply via ReviewNB

matteocao · 2022-09-30T13:29:20Z

examples/basic_tutorial_BERT.ipynb

@@ -0,0 +1,1198 @@
+{


if these two cells are "almost" duplicated, can you please create a function in one cell and call it with different parameters?

Probably same comments may work a few cells above

Reply via ReviewNB

matteocao · 2022-09-30T13:29:20Z

examples/basic_tutorial_BERT.ipynb

@@ -0,0 +1,1198 @@
+{


typo: At such --> As such

Reply via ReviewNB

Good catch!

matteocao · 2022-09-30T13:29:20Z

examples/basic_tutorial_BERT.ipynb

@@ -0,0 +1,1198 @@
+{


maybe one sentence describe the two cells below.

Reply via ReviewNB

matteocao · 2022-09-30T13:29:20Z

examples/basic_tutorial_BERT.ipynb

@@ -0,0 +1,1198 @@
+{


ca you clarify why you compute the gradients (and of what) in a sentence in a .md cell above this one?

Reply via ReviewNB

Added some discussion on this. Also: If you know of a more giotto-like way of getting your hands on these gradients I think it would improve this notebook.

hkirvesl · 2022-10-01T10:26:21Z

Thanks for the good comments! I pulled the main and it did fix a lot, (and also managed to close this PR.) Will reopen once I've cleaned the notebook more a bit more as per the suggestions

raphaelreinauer · 2022-10-03T08:20:42Z

Could you please install pre-commit (see README.md)

hkirvesl · 2022-10-03T09:50:26Z

Reopening the PR

Changelog:

The notebook:

Imports

1.1) Removed unneeded imports:
from gdeep.visualisation import persistence_diagrams_of_activations
from gdeep.visualisation import Visualiser

1.2) Added import

from gdeep.utility import DEVICE

Cleaned some variable names
Added more explanation as per the suggested comments
Removed unnecessary code chunks
renamed variables
Added functions to reduce redundant code
Add legend to a persistence diagram plot.

Testing:

Run all the prehooks, both black and mypy

models/utils.py

Replace the modification to retain the existing functionality

matteocao

this loks good and it is very interesting btw! thanks @hkirvesl !!

hkirvesl and others added 4 commits September 29, 2022 09:40

add support and demo for huggingface BERT

8521e5b

add support and demo for huggingface BERT

c5489e7

clean BERT tutorial

412c142

Merge branch 'giotto-ai:master' into hkdev

2b09c8c

matteocao requested changes Sep 29, 2022

View reviewed changes

hkirvesl and others added 6 commits September 29, 2022 18:59

fix jupyter nb metadata

fa1b481

Merge branch 'hkdev' of https://github.com/hkirvesl/giotto-deep into …

8fbbe57

…hkdev pull the test fixes

fix data loading to pass the CI

f70f562

there was a mismatch in where data was downloaded and read from, causing the CI to fail.

change pathdef to use os.path.join

3a39246

use os.path.join

c799be1

fix ModelExtractor BERT support

86962b5

matteocao requested changes Sep 30, 2022

View reviewed changes

matteocao reviewed Sep 30, 2022

View reviewed changes

hkirvesl closed this Oct 1, 2022

hkirvesl force-pushed the hkdev branch from 86962b5 to 89a0944 Compare October 1, 2022 06:48

hkirvesl added 2 commits October 1, 2022 09:08

merge recent fixes to main branch, eg precommits

8120be9

add fixes from black precommit hook

55b510c

Clean up BERT notebook

988d172

hkirvesl reopened this Oct 3, 2022

matteocao approved these changes Oct 4, 2022

View reviewed changes

matteocao merged commit f83811f into giotto-ai:master Oct 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add NB for BERT models #114

Add NB for BERT models #114

hkirvesl commented Sep 29, 2022 •

edited

Loading

review-notebook-app bot commented Sep 29, 2022

matteocao left a comment

matteocao Sep 29, 2022

hkirvesl Sep 29, 2022

matteocao Sep 29, 2022

hkirvesl Sep 30, 2022

matteocao left a comment

matteocao Sep 30, 2022

raphaelreinauer Sep 30, 2022

raphaelreinauer Sep 30, 2022

hkirvesl Oct 3, 2022

matteocao Sep 30, 2022

hkirvesl Oct 3, 2022

matteocao Sep 30, 2022

matteocao Sep 30, 2022

hkirvesl Oct 3, 2022

matteocao Sep 30, 2022

hkirvesl Oct 3, 2022

matteocao Sep 30, 2022

matteocao Sep 30, 2022

hkirvesl Oct 3, 2022

matteocao Sep 30, 2022

matteocao Sep 30, 2022

matteocao Sep 30, 2022

hkirvesl Oct 3, 2022

matteocao Sep 30, 2022

matteocao Sep 30, 2022

hkirvesl Oct 3, 2022

hkirvesl commented Oct 1, 2022

raphaelreinauer commented Oct 3, 2022

hkirvesl commented Oct 3, 2022

matteocao left a comment

Add NB for BERT models #114

Add NB for BERT models #114

Conversation

hkirvesl commented Sep 29, 2022 • edited Loading

review-notebook-app bot commented Sep 29, 2022

matteocao 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

matteocao 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

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

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

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

hkirvesl commented Oct 1, 2022

raphaelreinauer commented Oct 3, 2022

hkirvesl commented Oct 3, 2022

matteocao left a comment

Choose a reason for hiding this comment

hkirvesl commented Sep 29, 2022 •

edited

Loading