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

[RFC] Deprecate on_configure_sharded_model callback hook #11560

Closed
daniellepintz opened this issue Jan 20, 2022 · 11 comments · Fixed by #11627
Closed

[RFC] Deprecate on_configure_sharded_model callback hook #11560

daniellepintz opened this issue Jan 20, 2022 · 11 comments · Fixed by #11627
Assignees
Labels
deprecation Includes a deprecation good first issue Good for newcomers help wanted Open to be worked on hooks Related to the hooks API
Milestone

Comments

@daniellepintz
Copy link
Contributor

Proposed refactor

Deprecate on_configure_sharded_model callback/LM hooks in v1.6 for removal in v1.8
https://github.com/PyTorchLightning/pytorch-lightning/blob/5ad5ba54c0c477546d21daf75ac7b4748d7963a7/pytorch_lightning/callbacks/base.py#L60-L61

Motivation

I see no uses of this hook either externally or internally: https://www.grep.app/search?q=on_configure_sharded_model

Additional context

This hook was originally added in #6679

cc @justusschock @awaelchli @akihironitta @rohitgr7 @tchaton @carmocca @Borda @ninginthecloud @daniellepintz

@daniellepintz daniellepintz added deprecation Includes a deprecation hooks Related to the hooks API labels Jan 20, 2022
@daniellepintz daniellepintz changed the title Deprecate on_configure_sharded_model callback/LM hooks [RFC] Deprecate on_configure_sharded_model callback/LM hooks Jan 20, 2022
@tchaton
Copy link
Contributor

tchaton commented Jan 21, 2022

@SeanNaren I believe it has never been properly advertised. I think it would be fair to remove it as I don't really find a real use case for it.

@carmocca
Copy link
Member

I agree with removal, the hook is really specific. Using it doesn't paint a good picture design-wise so I think it'll be good to clean up, now that it's not necessary.

@daniellepintz daniellepintz added good first issue Good for newcomers help wanted Open to be worked on labels Jan 21, 2022
@daniellepintz
Copy link
Contributor Author

daniellepintz commented Jan 21, 2022

For anyone picking up this issue, you can follow #10940 as an example

@Piyush-97
Copy link
Contributor

I will try to do this.

@awaelchli
Copy link
Member

Yes!

@Piyush-97
Copy link
Contributor

I don't know how to write the test for this.

@ananthsub ananthsub changed the title [RFC] Deprecate on_configure_sharded_model callback/LM hooks [RFC] Deprecate on_configure_sharded_model callback hook Jan 25, 2022
@rohitgr7
Copy link
Contributor

hey @Piyush-97
for the test you need to ensure that the deprecation warning is caught when someone uses this hook.
check this for reference.

@Piyush-97
Copy link
Contributor

Piyush-97 commented Jan 25, 2022

`def test_v1_8_0_on_configure_sharded_model(tmpdir):
class TestCallback(Callback):
def on_configure_sharded_model(self, trainer, model):
print("Configuring sharded model")

model = BoringModel()

trainer = Trainer(
    callbacks=[TestCallback()],
    max_epochs=1,
    fast_dev_run=True,
    enable_progress_bar=False,
    logger=False,
    default_root_dir=tmpdir,
    strategy='ddp_sharded'
)
with pytest.deprecated_call(
    match="The `on_configure_sharded_model` callback hook was deprecated in v1.6 and will be removed in v1.8."
):
    trainer.fit(model)`

I don't have GPU and I am not sure if this is right.

@ananthsub
Copy link
Contributor

@Piyush-97 - you don't have to specify strategy="ddp_sharded" for the test. we should warn the user whenever the hook is overridden that it is deprecated.

def test_v1_8_0_on_configure_sharded_model(tmpdir):
    class TestCallback(Callback):
        def on_configure_sharded_model(self, trainer, model):
            print("Configuring sharded model")

    model = BoringModel()
    trainer = Trainer(
        callbacks=[TestCallback()],
        max_epochs=1,
        fast_dev_run=True,
        enable_progress_bar=False,
        logger=False,
        default_root_dir=tmpdir,
    )
    with pytest.deprecated_call(
        match="The `on_configure_sharded_model` callback hook was deprecated in v1.6 and will be removed in v1.8."
    ):
        trainer.fit(model)`

@Piyush-97
Copy link
Contributor

I have written a test but it is failing with two more tests.

@daniellepintz
Copy link
Contributor Author

Hi @Piyush-97 those failures are due to a known issue affecting multiple PRs, not just yours: #11633

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation Includes a deprecation good first issue Good for newcomers help wanted Open to be worked on hooks Related to the hooks API
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants