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

Low precision groupnorm #1976

Merged

Conversation

mvpatel2000
Copy link
Contributor

@mvpatel2000 mvpatel2000 commented Feb 16, 2023

What does this PR do?

Adds low precision groupnorm. This is modeled after low precision layernorm and is useful for stable diffusion. We've seen about +5% throughput and memory savings with this with no significant impact to loss curves.

What issue(s) does this change relate to?

CO-1794

@dskhudia
Copy link
Contributor

dskhudia commented Feb 16, 2023

Thoughs on combining this with lowprecisonlayernorm and calling that lowPrecisionNorms?

@mvpatel2000
Copy link
Contributor Author

Thoughs on combining this with lowprecisonlayernorm and calling that lowPrecisionNorms?

I would prefer to keep separate because we don't have guarantees of convergence with this, and there might be a case where one applies but another does not. While we have never observed issues with this, it is possible there are problems given AMP does not do this by default

@dskhudia
Copy link
Contributor

We can allow turning on/off each type based on an option.

@nik-mosaic
Copy link
Contributor

Is this scriptable? Can you export a model with LowPrecisionGroupnorm using torch.jit.script?

Copy link
Contributor

@dblalock dblalock left a comment

Choose a reason for hiding this comment

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

I have no context on this PR, so I just went through and found ways to polish it.

tests/common/models.py Outdated Show resolved Hide resolved
tests/algorithms/test_low_precision_groupnorm.py Outdated Show resolved Hide resolved
tests/algorithms/test_low_precision_groupnorm.py Outdated Show resolved Hide resolved
tests/algorithms/test_low_precision_groupnorm.py Outdated Show resolved Hide resolved
composer/algorithms/low_precision_groupnorm/README.md Outdated Show resolved Hide resolved
composer/algorithms/low_precision_groupnorm/README.md Outdated Show resolved Hide resolved
composer/algorithms/low_precision_groupnorm/README.md Outdated Show resolved Hide resolved
composer/algorithms/low_precision_groupnorm/__init__.py Outdated Show resolved Hide resolved
@mvpatel2000 mvpatel2000 requested a review from dblalock February 23, 2023 19:53
@mvpatel2000
Copy link
Contributor Author

Will switch to SimpleConvModel once #1991 merges

Copy link
Contributor

@nik-mosaic nik-mosaic left a comment

Choose a reason for hiding this comment

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

Two comments about the arguments and export. Once those are changed, I will approve.

@mvpatel2000
Copy link
Contributor Author

I need a different logo... any suggestions for icon

Copy link
Contributor

@dblalock dblalock left a comment

Choose a reason for hiding this comment

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

basically LGTM, except maybe one test needs to say "GroupNorm" instead of "LayerNorm"

composer/algorithms/low_precision_groupnorm/README.md Outdated Show resolved Hide resolved
composer/algorithms/low_precision_groupnorm/README.md Outdated Show resolved Hide resolved
composer/algorithms/low_precision_groupnorm/README.md Outdated Show resolved Hide resolved
composer/algorithms/low_precision_groupnorm/README.md Outdated Show resolved Hide resolved
composer/algorithms/low_precision_layernorm/README.md Outdated Show resolved Hide resolved
tests/algorithms/test_low_precision_groupnorm.py Outdated Show resolved Hide resolved
@mvpatel2000 mvpatel2000 merged commit 228efab into mosaicml:dev Feb 25, 2023
@mvpatel2000 mvpatel2000 deleted the mvpatel2000/low-precision-groupnorm branch February 25, 2023 05:28
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