Skip to content

fixing member reference #831

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.

Sign up for GitHub

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

fixing member reference #831

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c97f04b
fixing member reference
azrael417 Mar 31, 2025
ac7e857
remove dbg print
azrael417 Mar 31, 2025
78cee30
more reference fixing
azrael417 Mar 31, 2025
a80650f
some int casts for env variables
azrael417 Mar 31, 2025
dfac846
printing debug
azrael417 Mar 31, 2025
2adbb0d
printing debug
azrael417 Mar 31, 2025
291c016
removing debug prints
azrael417 Mar 31, 2025
168dbfb
Merge branch 'main' into tkurth/distributed-wireup-fix
coreyjadams Mar 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions physicsnemo/distributed/manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Copy link
Collaborator

@akshaysubr akshaysubr Mar 31, 2025

Choose a reason for hiding this comment

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

Isn't this int conversion handled in the next if block? And won't this break when LOCAL_RANK is None?

Copy link
Contributor Author

@azrael417 azrael417 Apr 1, 2025

Choose a reason for hiding this comment

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

can that happen? if LOCAL_RANK is set, can it still be None: like with

export LOCAL_RANK=

?

if local_rank is not None:
local_rank = int(local_rank)
else:
Expand Down Expand Up @@ -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"
Copy link
Collaborator

@akshaysubr akshaysubr Mar 31, 2025

Choose a reason for hiding this comment

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

Any specific reason to switch to using private attributes here instead of the public property versions of these as was used before?

Copy link
Contributor Author

@azrael417 azrael417 Apr 1, 2025

Choose a reason for hiding this comment

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

If the external ones alias those internal ones, that is OK, but isn't it cleaner to use the internal ones for internal tasks?

)

if manager._distributed:
# Setup distributed process group
try:
dist.init_process_group(
backend,
rank=manager.rank,
world_size=manager.world_size,
device_id=manager.device,
rank=manager._rank,
world_size=manager._world_size,
device_id=manager._device,
)
except TypeError:
# device_id only introduced in PyTorch 2.3
dist.init_process_group(
backend,
rank=manager.rank,
world_size=manager.world_size,
rank=manager._rank,
world_size=manager._world_size,
)

if torch.cuda.is_available():
# Set device for this process and empty cache to optimize memory usage
torch.cuda.set_device(manager.device)
torch.cuda.device(manager.device)
torch.cuda.set_device(manager._device)
torch.cuda.device(manager._device)
torch.cuda.empty_cache()

manager._initialization_method = method
Expand Down