Skip to content
This repository has been archived by the owner on Jul 1, 2024. It is now read-only.

Make blocks attachable automatically when needed #461

Closed

Conversation

mannatsingh
Copy link
Contributor

Summary:
I was frustrated by the fact that I needed to change the code for every model if I needed to be able to attach heads to it - something we need to do with most trained models.
Ideally, users should be able to write their models without writing anything classy vision specific and be able to attach heads.

I've made a couple of diffs which get us there, wanted to see what everyone thought about them. Please look at the second diff to see the end result.

In this diff, I make the following changes -

  • build_attachable_block becomes a private function (_build_attachable_block), and models don't need to call it anymore.
  • Instead, when someone tries to attach to a module called my_block, we recursively search for it and wrap it by a ClassyBlock on the fly
  • Updated get_classy_state and set_classy_state to be compatible with the changes
  • Users can attach heads to any block which has a unique name (like block3-2)
  • models_classy_model_test wasn't being run internally; renamed
    classy_block_test to models_classy_block_test
  • Added additional test cases to test the changes

NOTE: This breaks all checkpoints since the model definitions have changed.

Differential Revision: D20714865

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported labels Mar 31, 2020
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D20714865

mannatsingh added a commit to mannatsingh/ClassyVision that referenced this pull request Apr 1, 2020
Summary:
Pull Request resolved: facebookresearch#461

I was frustrated by the fact that I needed to change the code for every model if I needed to be able to attach heads to it - something we need to do with most trained models.
Ideally, users should be able to write their models without writing anything classy vision specific and be able to attach heads.

I've made a couple of diffs which get us there, wanted to see what everyone thought about them. Please look at the second diff to see the end result.

In this diff, I make the following changes -
- `build_attachable_block` becomes a private function (`_build_attachable_block`), and models  don't need to call it anymore.
- Instead, when someone tries to attach to a module called `my_block`, we recursively search for it and wrap it by a `ClassyBlock` on the fly
- Updated `get_classy_state` and `set_classy_state` to be compatible with the changes
- Users can attach heads to any block which has a unique name (like `block3-2`)
- `models_classy_model_test` wasn't being run internally; renamed
`classy_block_test` to `models_classy_block_test`
- Added additional test cases to test the changes

NOTE: This breaks all checkpoints since the model definitions have changed.

Differential Revision: D20714865

fbshipit-source-id: 95c1ce4655663538919ca28f77f554ea67570edb
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D20714865

mannatsingh added a commit to mannatsingh/ClassyVision that referenced this pull request Apr 1, 2020
Summary:
Pull Request resolved: facebookresearch#461

I was frustrated by the fact that I needed to change the code for every model if I needed to be able to attach heads to it - something we need to do with most trained models.
Ideally, users should be able to write their models without writing anything classy vision specific and be able to attach heads.

I've made a couple of diffs which get us there, wanted to see what everyone thought about them. Please look at the second diff to see the end result.

In this diff, I make the following changes -
- `build_attachable_block` becomes a private function (`_build_attachable_block`), and models  don't need to call it anymore.
- Removed `_should_cache_output` and `set_cache_output` from `ClassyBlock`
- Added a redundant `_attachable_block_names` attribute - this is needed for reading the block names inside torch script (`_attachable_blocks` is inaccessible) - T64918869
- Instead, when someone tries to attach to a module called `my_block`, we recursively search for it and wrap it by a `ClassyBlock` on the fly
- Updated `get_classy_state` and `set_classy_state` to be compatible with the changes
- Users can attach heads to any block which has a unique name (like `block3-2`)
- `models_classy_model_test` wasn't being run internally; renamed
`classy_block_test` to `models_classy_block_test`
- Added additional test cases to test the changes

NOTE: This breaks all checkpoints since the model definitions have changed. We still handle old checkpoints for `ResNeXt` models to allow for a smoother transition -  T61141249

Differential Revision: D20714865

fbshipit-source-id: f48e768aede1c10d25754f0fad0f24ccac9a1503
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D20714865

mannatsingh added a commit to mannatsingh/ClassyVision that referenced this pull request Apr 1, 2020
Summary:
Pull Request resolved: facebookresearch#461

I was frustrated by the fact that I needed to change the code for every model if I needed to be able to attach heads to it - something we need to do with most trained models.
Ideally, users should be able to write their models without writing anything classy vision specific and be able to attach heads.

I've made a couple of diffs which get us there, wanted to see what everyone thought about them. Please look at the second diff to see the end result.

In this diff, I make the following changes -
- `build_attachable_block` becomes a private function (`_build_attachable_block`), and models  don't need to call it anymore.
- Removed `_should_cache_output` and `set_cache_output` from `ClassyBlock`
- Added a redundant `_attachable_block_names` attribute - this is needed for reading the block names inside torch script (`_attachable_blocks` is inaccessible) - T64918869
- Instead, when someone tries to attach to a module called `my_block`, we recursively search for it and wrap it by a `ClassyBlock` on the fly
- Updated `get_classy_state` and `set_classy_state` to be compatible with the changes
- Users can attach heads to any block which has a unique name (like `block3-2`)
- `models_classy_model_test` wasn't being run internally; renamed
`classy_block_test` to `models_classy_block_test`
- Added additional test cases to test the changes

NOTE: This breaks all checkpoints since the model definitions have changed. We still handle old checkpoints for `ResNeXt` models to allow for a smoother transition -  T61141249

Reviewed By: vreis

Differential Revision: D20714865

fbshipit-source-id: 993a841b7190d5fabd751d8cca25381dcd93d898
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D20714865

Summary:
Pull Request resolved: facebookresearch#461

I was frustrated by the fact that I needed to change the code for every model if I needed to be able to attach heads to it - something we need to do with most trained models.
Ideally, users should be able to write their models without writing anything classy vision specific and be able to attach heads.

I've made a couple of diffs which get us there, wanted to see what everyone thought about them. Please look at the second diff to see the end result.

In this diff, I make the following changes -
- `build_attachable_block` becomes a private function (`_build_attachable_block`), and models  don't need to call it anymore.
- Removed `_should_cache_output` and `set_cache_output` from `ClassyBlock`
- Added a redundant `_attachable_block_names` attribute - this is needed for reading the block names inside torch script (`_attachable_blocks` is inaccessible) - T64918869
- Instead, when someone tries to attach to a module called `my_block`, we recursively search for it and wrap it by a `ClassyBlock` on the fly
- Updated `get_classy_state` and `set_classy_state` to be compatible with the changes
- Users can attach heads to any block which has a unique name (like `block3-2`)
- `models_classy_model_test` wasn't being run internally; renamed
`classy_block_test` to `models_classy_block_test`
- Added additional test cases to test the changes

NOTE: This breaks all checkpoints since the model definitions have changed. We still handle old checkpoints for `ResNeXt` models to allow for a smoother transition -  T61141249

Reviewed By: vreis

Differential Revision: D20714865

fbshipit-source-id: d3cd867886ad857efe31442458da761418a51d6d
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D20714865

facebook-github-bot pushed a commit that referenced this pull request Apr 9, 2020
Summary:
Pull Request resolved: #461

I was frustrated by the fact that I needed to change the code for every model if I needed to be able to attach heads to it - something we need to do with most trained models.
Ideally, users should be able to write their models without writing anything classy vision specific and be able to attach heads.

I've made a couple of diffs which get us there, wanted to see what everyone thought about them. Please look at the second diff to see the end result.

In this diff, I make the following changes -
- `build_attachable_block` becomes a private function (`_build_attachable_block`), and models  don't need to call it anymore.
- Removed `_should_cache_output` and `set_cache_output` from `ClassyBlock`
- Added a redundant `_attachable_block_names` attribute - this is needed for reading the block names inside torch script (`_attachable_blocks` is inaccessible) - T64918869
- Instead, when someone tries to attach to a module called `my_block`, we recursively search for it and wrap it by a `ClassyBlock` on the fly
- Updated `get_classy_state` and `set_classy_state` to be compatible with the changes
- Users can attach heads to any block which has a unique name (like `block3-2`)
- `models_classy_model_test` wasn't being run internally; renamed
`classy_block_test` to `models_classy_block_test`
- Added additional test cases to test the changes

NOTE: This breaks all checkpoints since the model definitions have changed. We still handle old checkpoints for `ResNeXt` models to allow for a smoother transition -  T61141249

Reviewed By: vreis

Differential Revision: D20714865

fbshipit-source-id: f8fa269b5deddbdd77461047850e7050b1bc921a
@mannatsingh mannatsingh deleted the export-D20714865 branch June 29, 2020 21:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants