FEA: Distributed Recommendation Implemention #1338
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Ethan-TZ
requested review from
hyp1231,
chenyushuo,
2017pxy,
Wicknight,
Sherry-XLL and
leoleojie
hyp1231
requested changes
Jul 7, 2022
recbole/config/configurator.py
Outdated
Comment on lines
344
to
345
| gpu_list = self.final_config_dict['gpu_ids'] | ||
| os.environ["CUDA_VISIBLE_DEVICES"] = ','.join(gpu_list) |
There was a problem hiding this comment.
- I suggest not changing the name of a widely-used arg like
gpu_id. In my opinion, it's OK to usegpu_ideven for multiple GPU IDs. - When input multiple GPU IDs, maybe it's better to use
gpu_id: "1,2,3,4"(as a string) rather thangpu_id: [1, 2, 3, 4](as a List). As users may input this arg via command line, such aspython run_recbole.py --gpu_id=1,2,3,4., and the List may be difficult to input via command line. - Better to assign an initial value for
gpu_id, or users have to specify an additional arg whenever they want to run.
Suggested change
| gpu_list = self.final_config_dict['gpu_ids'] | |
| os.environ["CUDA_VISIBLE_DEVICES"] = ','.join(gpu_list) | |
| gpu_list = self.final_config_dict['gpu_id'] | |
| os.environ["CUDA_VISIBLE_DEVICES"] = gpu_list |
recbole/config/configurator.py
Outdated
| self.final_config_dict['device'] = torch.device("cuda" if torch.cuda.is_available() and use_gpu else "cpu") | ||
| gpu_list = self.final_config_dict['gpu_ids'] | ||
| os.environ["CUDA_VISIBLE_DEVICES"] = ','.join(gpu_list) | ||
| self.final_config_dict['SingleSpec'] = True |
There was a problem hiding this comment.
Seems that most existing arg names follow a style like single_spec. Please feel free to point out if there are some specific concerns about the naming styles.
Besides, will it be more clear if we move this line after if 'local_rank' not in self.final_config_dict: and before else:?
| @@ -16,7 +16,6 @@ | |||
| import os | |||
| import sys | |||
| import yaml | |||
| import torch | |||
There was a problem hiding this comment.
Seems that regardless of the existence of local_rank, we need to import torch. So what are the concerns of removing this line here?
There was a problem hiding this comment.
In order to make the setting of environment variables effective, we must put the os.environ["CUDA_VISIBLE_DEVICES"] behind import torch.
recbole/config/configurator.py
Outdated
Comment on lines
355
to
356
| torch.distributed.init_process_group(backend='nccl', rank = self.final_config_dict['local_rank'], world_size = self.final_config_dict['world_size'], | ||
| init_method='tcp://' + self.final_config_dict['ip'] + ':' + str(self.final_config_dict['port'])) |
There was a problem hiding this comment.
Please take care of the coding style. [PEP8]
Suggested change
| torch.distributed.init_process_group(backend='nccl', rank = self.final_config_dict['local_rank'], world_size = self.final_config_dict['world_size'], | |
| init_method='tcp://' + self.final_config_dict['ip'] + ':' + str(self.final_config_dict['port'])) | |
| torch.distributed.init_process_group( | |
| backend='nccl', rank=self.final_config_dict['local_rank'], | |
| world_size=self.final_config_dict['world_size'], | |
| init_method='tcp://' + self.final_config_dict['ip'] + ':' + str(self.final_config_dict['port'])) |
recbole/config/configurator.py
Outdated
| if 'local_rank' not in self.final_config_dict: | ||
| import torch | ||
| self.final_config_dict['local_rank'] = 0 | ||
| self.final_config_dict['device'] = torch.device("cpu") if len(gpu_list) == 0 else torch.device("cuda") |
There was a problem hiding this comment.
Better to check with torch.cuda.is_available()?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.