Use hydra.run.dir (not os.getcwd) for DDP subprocesses' run dir

[nisheethlahoti]

What does this PR do?

Fixes #16694

Before submitting

• Was this discussed/agreed via a GitHub issue? (not for typos and docs)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure your PR does only one thing, instead of bundling different changes together?
• Did you make sure to update the documentation with your changes? (if necessary)
• Did you write any new necessary tests? (not for typos and docs)
• Did you verify new and existing tests pass locally with your changes?
• Did you list all the breaking changes introduced by this pull request?
• Did you update the CHANGELOG? (not for typos, docs, test updates, or minor internal changes/refactors)

PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the review guidelines. In short, see the following bullet-list:

Reviewer checklist

• Is this pull request ready for review? (if not, please submit in draft mode)
• Check that all items from Before submitting are resolved
• Make sure the title is self-explanatory and the description concisely explains the PR
• Add labels and milestones (and optionally projects) to the PR so it can be classified

[codecov]

Codecov Report

Merging #18145 (a4457f1) into master (3d573d5) will decrease coverage by 22%.
The diff coverage is 0%.

❗ Current head a4457f1 differs from pull request most recent head 65caaa5. Consider uploading reports for the commit 65caaa5 to get more accurate results

Additional details and impacted files

@@            Coverage Diff            @@
##           master   #18145     +/-   ##
=========================================
- Coverage      83%      61%    -22%     
=========================================
  Files         431      426      -5     
  Lines       32799    32710     -89     
=========================================
- Hits        27364    19936   -7428     
- Misses       5435    12774   +7339     

[nisheethlahoti]

All the failing tests are failing with "ModuleNotFound: torch", which cannot possibly be caused by this PR.
Locally, the PR is running successfully.

[ishandutta0098]

@aniketmaurya can you look into this once? We discussed this in our call yesterday.

[awaelchli]

@nisheethlahoti Thanks for the PR. The change here looks good overall, but we need to make sure we don't regress on the issue here: #15689
There was a PR a while ago that change the behavior of the launcher wrt. Hydra too (#11617) but we had to revert it (see discussion here #15689).

If you would be so kind to go over these two issues to check whether there are any conflicts between your approach and the concerns raised there. That would be very helpful (forgive me, I'm a bit unfamiliar with hydra).

All the failing tests are failing with "ModuleNotFound: torch", which cannot possibly be caused by this PR.

Don't worry about this, we will take care of it once the PR can be merged :)

[nisheethlahoti]

If you would be so kind to go over these two issues to check whether there are any conflicts between your approach and the concerns raised there.

The behaviour defined in #11617 actually seems better than what's proposed here (since it handles multi-run properly and has separate subdirectories for all the different PL processes), with 1 exception: that it actually tries to read the config from the .hydra directory (the problem raised in #15689). This PR doesn't have that issue since it doesn't try to read any stored config (It accesses run.dir through HydraConfig.get(), which is present whether or not a config is stored).

However, this could also be fixed by reinstating the code in #11617, removing the line command += ["-cp", hydra_output, "-cn", "config.yaml"], and running the new commands with cwd=get_original_cwd() like in #15737. This would add multirun support again, while not breaking the NeMo use-case, IMO. Should I modify this PR (or send a new one) to do that instead?

[nisheethlahoti]

Update to previous comment: Removing the -cp hydra_output line will actually disable multi-run support, but we will still have:

1. Creation of different subdirectories for all spawned processes.
2. Support for multi-run if user has enabled hydra's experimental-rerun callback

[awaelchli]

Thanks @nisheethlahoti for your helpful analysis! Based on this I suggest we move forward with your PR as is.

However, this could also be fixed by reinstating the code in #11617 [...] This would add multirun support again, while not breaking the NeMo use-case, IMO. Should I modify this PR (or send a new one) to do that instead?

This is good news. I would suggest that we break it in two parts, this PR + multi-run in a different PR.

[awaelchli]

Would it be possible to add a small unit test in tests/tests_fabric/strategies/launchers/test_subprocess_script.py? Happy to help out if needed.

[nisheethlahoti]

Would it be possible to add a small unit test in tests/tests_fabric/strategies/launchers/test_subprocess_script.py? Happy to help out if needed.

Done

[awaelchli]

Awesome! Thank you @nisheethlahoti 🎉