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

test for ensuring hydra ddp raises is raising for the wrong reason #11

Closed
rsokl opened this issue Apr 14, 2022 · 3 comments
Closed

test for ensuring hydra ddp raises is raising for the wrong reason #11

rsokl opened this issue Apr 14, 2022 · 3 comments
Assignees
Labels
bug Something isn't working test-suite

Comments

@rsokl
Copy link
Contributor

rsokl commented Apr 14, 2022

@jgbos

In the following test:

@pytest.mark.usefixtures("cleandir")
def test_ddp_with_hydra_raises():
trainer = builds(
Trainer,
max_epochs=1,
accelerator="auto",
devices="${devices}",
strategy=builds(HydraDDP),
fast_dev_run=True,
)
module = builds(TestLightningModule)
Config = make_config(trainer=trainer, wrong_config_name=module, devices=2)
with pytest.raises(TypeError):
launch(Config, pl_main_task)

launch(Config, pl_main_task) raises TypeError because pl_main_task doesn't accept a single config (pyright warned me about this). I doubt this is what you meant to exercise in this test.

I am confused about what this test is doing. Config = make_config(trainer=trainer, wrong_config_name=module, devices=2) makes it seem like we are making sure that launch fails for a config with a bad field name, but the test seems like should be exercising ddp

@rsokl rsokl added bug Something isn't working test-suite labels Apr 14, 2022
@rsokl rsokl changed the title hydra ddp test is raising for the wrong reason test for ensuring hydra ddp raises is raising for the wrong reason Apr 14, 2022
@jgbos
Copy link
Contributor

jgbos commented Apr 15, 2022

Ok, I'll take a look at this. Not sure we need this exact test, but instead need some way to raise an error that is useful to the user when using HydraDDP since it requires that the user set configs for trainer, module, and for PL 1.6 pl_testing (automatic for 1.5). Here I was just making sure it raises the error.

@jgbos
Copy link
Contributor

jgbos commented Apr 15, 2022

Just to clarify, I'm not sure I can test for an error being thrown in the subprocess call in HydraDDP, so I wanted to make sure it would throw an error. Unfortunately if it throws an error in the subprocess I think the process will just hang.

jgbos added a commit that referenced this issue Apr 16, 2022
@jgbos
Copy link
Contributor

jgbos commented Apr 16, 2022

The tests are fixed but I need to think of way to catch the possible configuration error in the subprocess.

@rsokl rsokl closed this as completed in b697aeb Apr 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working test-suite
Projects
None yet
Development

No branches or pull requests

2 participants