Skip to content
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

Optimize Bert_SQUAD_Interpret #672

Closed

Conversation

NarineK
Copy link
Contributor

@NarineK NarineK commented May 25, 2021

Optimizes Bert_SQUAD_Interpret tutorial by:

  1. Using LayerIG for multiple layers for all embedding types
  2. Removes dependancies from configure_interpretable_layer by using forward function with the inputs_embeds argument.

Addresses the improvements discussed in: #624

@@ -40,7 +40,7 @@ conda install -y -c conda-forge black matplotlib pytest-cov sphinx-autodoc-typeh
# install node/yarn for insights build
conda install -y -c conda-forge yarn
# nodejs should be last, otherwise other conda packages will downgrade node
conda update -y --no-channel-priority -c conda-forge nodejs
conda install -y --no-channel-priority -c conda-forge nodejs=13
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vivekmig, @edward-io , this fixes the issue with the nodejs version. There might be better fixes but this fixed it for now.
Issue: #669

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we peg this to nodejs=14? According to https://nodejs.org/en/about/releases/ NodeJS 14 will be supported until 2022.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @edward-io! Created a separate PR (#675) so that we so that we don't have to wait Bert tutorial to be reviewed.

@edward-io edward-io self-requested a review May 25, 2021 20:27
Copy link
Contributor

@edward-io edward-io left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks for fixing this.

@NarineK NarineK mentioned this pull request May 26, 2021
facebook-github-bot pushed a commit that referenced this pull request May 26, 2021
Summary:
Creating separate PR so that we can merge it quicker and don't have to wait for #672 to be reviewed.

Pull Request resolved: #675

Reviewed By: bilalsal

Differential Revision: D28713735

Pulled By: NarineK

fbshipit-source-id: 100694e691d98f3107e89f89e556dad5d0eabd73
Copy link
Contributor

@vivekmig vivekmig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great 👍 !

" input_embeddings = interpretable_embedding.indices_to_embeddings(input_ids, token_type_ids=token_type_ids, position_ids=position_ids)\n",
" ref_input_embeddings = interpretable_embedding.indices_to_embeddings(ref_input_ids, token_type_ids=token_type_ids, position_ids=position_ids)\n",
" input_embeddings = model.bert.embeddings(input_ids, token_type_ids=token_type_ids, position_ids=position_ids)\n",
" #interpretable_embedding.indices_to_embeddings(input_ids, token_type_ids=token_type_ids, position_ids=position_ids)\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Should these commented out lines be removed?

@NarineK NarineK force-pushed the narine/fix_bert_squad_tutorial branch from 8889939 to 3d767bd Compare June 21, 2021 22:26
@facebook-github-bot
Copy link
Contributor

@NarineK has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@NarineK merged this pull request in e281552.

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

Successfully merging this pull request may close these issues.

4 participants