Skip to content
This repository has been archived by the owner on Dec 16, 2022. It is now read-only.

upgrade to pytorch 1.2 #3182

Merged
merged 16 commits into from
Aug 24, 2019
Merged

upgrade to pytorch 1.2 #3182

merged 16 commits into from
Aug 24, 2019

Conversation

joelgrus
Copy link
Contributor

as you can see, this ended up not being that much work.

there are still two tests failing for me.

the first is the new "docstring" test, which I don't entirely understand:

E               AssertionError: 'usag[1066 chars]tps://\n                        allennlp.s3.am[1452 chars]de\n' != 'usag[1066 chars]tps:/\n                        /allennlp.s3.am[1452 chars]de\n'
E               Diff is 4277 characters long. Set self.maxDiff to None to see it. : The documentation for the subcommand usage in the module allennlp.commands.elmo does not match the output of running `allennlp elmo --help`. Please update the docstring to match the output.

the second is naqanet_test:

Parameter: _question_weights_predictor.bias had incorrect gradient: zeros with shape ((1,))

I'll keep digging into these, but if anyone has any ideas, let me know

@matt-gardner
Copy link
Contributor

For the naqanet test, first things to try are checking for dropout in the test fixture, and seeing if the test is just flaky. Other than that, not so many good ideas.

@DeNeutoy
Copy link
Contributor

Hmm i'll revert that docstring test. It looks broken and takes 60 seconds...

@joelgrus joelgrus changed the title WIP: upgrade to pytorch 1.2 upgrade to pytorch 1.2 Aug 22, 2019
@joelgrus
Copy link
Contributor Author

ok, this is ready for review, I guess. the one part I'm not happy with (and that you won't be happy with) is that I couldn't figure out a way to get the biaffine_dependency_parser_multilang_test to not trigger the zero gradients, so I had to add a gradients_to_ignore (which is just a single bias parameter) to those tests.

/~https://github.com/allenai/allennlp/pull/3182/files#diff-83e1bde8a01afa0c96f68ca171f3ba8bR11

if anyone has a better way to deal with that, I'm open to it.

Copy link
Contributor

@matt-gardner matt-gardner left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -101,7 +101,7 @@
packages=find_packages(exclude=["*.tests", "*.tests.*",
"tests.*", "tests"]),
install_requires=[
'torch>=0.4.1,<1.2',
'torch>=1.2.0',
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we strictly need features from 1.2.0 or is 1.0 sufficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this PR is in response to breaking changes that happened between 1.1 and 1.2, but also going forward we'd like to use PyTorch's dataloader infrastructure, and for that to support allennlp datasets we need torch>=1.2

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. Thanks for explaining.

@brendan-ai2
Copy link
Contributor

@TalSchuster, I believe you wrote the biaffine_dependency_parser_multilang code. Strangely one of the tests for that code is producing zero gradients when we upgrade to PyTorch 1.2. Any insight on why that might be? We're tempted to ignore, but it's a bit unsettling.

@joelgrus joelgrus merged commit 23efadd into allenai:master Aug 24, 2019
reiyw pushed a commit to reiyw/allennlp that referenced this pull request Nov 12, 2019
* first attempt at pytorch 1.2

* explicit is better than implicit

* more explicit

* attempt to fix flaky tests

* pylint

* no disable dropout

* disable dropout by default

* restore dropout, don't deepcopy

* change batch size for biaffine_dependency_parser_multilang_test, maybe that will make it pass? :(

* try batch size 10

* ignore bad gradient parameter

* cleanup
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants