Skip to content

remove BC-breaking changes #1560

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

Merged
merged 5 commits into from
Nov 15, 2019
Merged

Conversation

eellison
Copy link
Contributor

@eellison eellison commented Nov 6, 2019

IntermediateLayerGetter and DenseBlock were re-written to be supported in script, along the way introducing BC changes that required _load_from_state_dict logic.

This removes those changes, moving them to ModuleDict. Once pytorch/pytorch#28988 lands we can re-enable these scripting tests.

@eellison eellison requested review from fmassa and driazati November 6, 2019 22:21
@driazati driazati changed the title remove changes that induced BC remove BC-breaking changes Nov 6, 2019
script_test_models = [
"deeplabv3_resnet101",
# "deeplabv3_resnet101", temp disabled
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is all going in before a release it's better to just wait until pytorch#28988 lands then land this PR so these tests never have to be disabled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK There's a minor release of torchvision coming up, this PR is for that release

Copy link
Member

Choose a reason for hiding this comment

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

The minor release of torchvision didn't contain the BC-breaking commits, so I think we can wait until pytorch#28988 lands to merge this PR, thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay cool, in that case yes we can wait.

Copy link
Member

Choose a reason for hiding this comment

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

@eellison pytorch/pytorch#28988 has landed, can you re-enable this test?

@codecov-io
Copy link

codecov-io commented Nov 7, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@e4ba602). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1560   +/-   ##
=========================================
  Coverage          ?   65.95%           
=========================================
  Files             ?       90           
  Lines             ?     7047           
  Branches          ?     1070           
=========================================
  Hits              ?     4648           
  Misses            ?     2094           
  Partials          ?      305
Impacted Files Coverage Δ
torchvision/models/densenet.py 82.67% <100%> (ø)
torchvision/models/_utils.py 90% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e4ba602...2c836d0. Read the comment docs.

Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot!

I think we can wait until pytorch#28988 lands and enable those tests, but I'm fine merging this as is now and then re-enable them afterwards

script_test_models = [
"deeplabv3_resnet101",
# "deeplabv3_resnet101", temp disabled
Copy link
Member

Choose a reason for hiding this comment

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

The minor release of torchvision didn't contain the BC-breaking commits, so I think we can wait until pytorch#28988 lands to merge this PR, thoughts?

@fmassa
Copy link
Member

fmassa commented Nov 15, 2019

Merging this as the failures are unrelated.

@fmassa fmassa merged commit 10a7111 into pytorch:master Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants