Skip to content

Use DS callable API to allow hf_scheduler + ds_optimizer - #13216

Merged
stas00 merged 11 commits into
huggingface:masterfrom
tjruwase:olruwase/callable_opt_scheduler
Aug 30, 2021
Merged

stas00 merged 11 commits into
huggingface:masterfrom
tjruwase:olruwase/callable_opt_scheduler

Conversation

@tjruwase

@tjruwase tjruwase commented Aug 21, 2021

Copy link
Copy Markdown

This PR:

  • Used the (new) Callable api of deepspeed.initialize() to enable combining hf schedulers with deepspeed optimizers.
  • create_scheduler now has an optional optimizer arg
  • Updates relevant unit test.

Blocking events: All unblocked now.

  • depends on deepspeed PR 1316.
  • needs new deepspeed version after PR is merged and need to update the dependencies when that happens.

deepspeed: @stas00.

@stas00 stas00 self-assigned this Aug 22, 2021

@stas00 stas00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for solving this limitation, @tjruwase

I just proposed 2 fixes to keep things back-compatible. Please let me know if it's still OK.

Comment thread src/transformers/trainer.py Outdated
Comment thread src/transformers/trainer.py Outdated

@stas00 stas00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We will have to wait for @sgugger to finalize this as we are messing with the API. I think he's back on the 30th. in a week.

Comment thread src/transformers/deepspeed.py
Comment thread src/transformers/trainer.py Outdated
Comment thread src/transformers/trainer.py Outdated

@stas00 stas00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for making the changes, Tunji. Looking good.

Now let's wait for the merge of your DS PR and the new release.

@stas00
stas00 requested a review from sgugger August 24, 2021 20:32
@jeffra

jeffra commented Aug 26, 2021

Copy link
Copy Markdown
Contributor

Comment thread src/transformers/trainer.py Outdated
Comment thread src/transformers/trainer.py

@sgugger sgugger left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for your PR!

I have no problem with optimizer becoming an optional argument of Trainer.create_scheduler, I am less convinced by the need to change the return type of the two methods Trainer.create_optimizer and Trainer.create_scheduler, though I don't have particularly strong feelings. I understand it's more convenient to do

return trainer.create_scheduler(xxx)

instead of

trainer.create_scheduler(xxx)
return trainer.lr_scheduler

but it's internal code.

It's just a thought, if you really prefer the change of return type, I'm fine with it.

Comment thread src/transformers/trainer.py Outdated
@stas00
stas00 merged commit 42f359d into huggingface:master Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants