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

Fixed gradient checkpoint bug for TimeSeriesTransformer #22272

Merged
merged 4 commits into from
Mar 23, 2023

Conversation

pmollerus23
Copy link
Contributor

@pmollerus23 pmollerus23 commented Mar 20, 2023

What does this PR do?

Moved gradient checkpointing clause to above the decoder layer implementation. This should fix the bug this issue addresses.

Fixes #21737

Before submitting

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@younesbelkada

Copy link
Contributor

@younesbelkada younesbelkada left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this! I left a tiny suggestion

Comment on lines 1122 to 1126
if use_cache:
logger.warning_once(
"`use_cache=True` is incompatible with gradient checkpointing. Setting `use_cache=False`..."
)
use_cache = False
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if use_cache:
logger.warning_once(
"`use_cache=True` is incompatible with gradient checkpointing. Setting `use_cache=False`..."
)
use_cache = False
if use_cache:
logger.warning_once(
"`use_cache=True` is incompatible with gradient checkpointing. Setting `use_cache=False`..."
)
use_cache = False

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, just fixed indentation with another commit. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome! Can you run make fixup to fix the styling checks? Then we should be good to go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! For some reason, I cannot seem to install this tool properly. Are there any guides on how to install this for the Transformers repository?

Copy link
Contributor

Choose a reason for hiding this comment

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

if you are inside transformers repo, you should be able to just run make fixup and it should work. What are the errors you get?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I run that, I get this output:

pmollerus@Philips-MacBook-Pro time_series_transformer % make fixup
make: *** No rule to make target `fixup'.  Stop.

I also tried running make fixup modeling_time_series_transformers.py in the time series transformer directory and receive the same output.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, can you set yourself on the root of transformers repo? i.e. where the Makefile lives, that should fix the issue I believe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doing this, I receive the following output:

pmollerus@Philips-MacBook-Pro transformers % make fixup
make: python: Command not found
No library .py files were modified
python utils/custom_init_isort.py
make: python: No such file or directory

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh this is because python is not installed on your environment I guess, I can run the command for you if you want, just let me know

Copy link
Contributor

Choose a reason for hiding this comment

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

Just pushed for you!

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Mar 20, 2023

The documentation is not available anymore as the PR was closed or merged.

Copy link
Contributor

@younesbelkada younesbelkada left a comment

Choose a reason for hiding this comment

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

Thanks a lot for all your work on this!

Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

@sgugger sgugger merged commit 5a9eb31 into huggingface:main Mar 23, 2023
@pmollerus23
Copy link
Contributor Author

Strange, I will see if I can install Python for my environment then. Thank you for all your help, and thanks for running make for me!

raghavanone pushed a commit to raghavanone/transformers that referenced this pull request Apr 5, 2023
…22272)

* Fixed gradient checkpoint bug for this model

* Updating PR indentation (maintainer feedback)

* make fixup

---------

Co-authored-by: younesbelkada <younesbelkada@gmail.com>
novice03 pushed a commit to novice03/transformers that referenced this pull request Jun 23, 2023
…22272)

* Fixed gradient checkpoint bug for this model

* Updating PR indentation (maintainer feedback)

* make fixup

---------

Co-authored-by: younesbelkada <younesbelkada@gmail.com>
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.

[Generate] Fix gradient_checkpointing and use_cache bug for generate-compatible models
4 participants