fixing member reference #831
Conversation
|
/blossom-ci |
There was a problem hiding this comment.
Casting local_rank to int before checking it's not-None seems like a good idea. What error were you encountering where it needed this?
Changing the access of rank/local_rank/world_size/device from the property access to private attributes seems counterintuitive. Can you elaborate on the reason for that change?
|
/blossom-ci |
| @@ -311,7 +311,7 @@ def initialize_env(): | |||
| rank = int(os.environ.get("RANK")) | |||
| world_size = int(os.environ.get("WORLD_SIZE")) | |||
| if "LOCAL_RANK" in os.environ: | |||
| local_rank = os.environ.get("LOCAL_RANK") | |||
| local_rank = int(os.environ.get("LOCAL_RANK")) | |||
There was a problem hiding this comment.
Isn't this int conversion handled in the next if block? And won't this break when LOCAL_RANK is None?
There was a problem hiding this comment.
can that happen? if LOCAL_RANK is set, can it still be None: like with
export LOCAL_RANK=
?
| @@ -548,30 +548,30 @@ def setup( | |||
| manager._local_rank = local_rank | |||
|
|
|||
| manager._device = torch.device( | |||
| f"cuda:{manager.local_rank}" if torch.cuda.is_available() else "cpu" | |||
| f"cuda:{manager._local_rank}" if torch.cuda.is_available() else "cpu" | |||
There was a problem hiding this comment.
Any specific reason to switch to using private attributes here instead of the public property versions of these as was used before?
There was a problem hiding this comment.
If the external ones alias those internal ones, that is OK, but isn't it cleaner to use the internal ones for internal tasks?
PhysicsNeMo Pull Request
Description
This fixes a wire error with new torch, because the wrong member variable is referenced in init process group, so the except branch is always triggered.
Checklist
Dependencies