Initial test of using gpu_topology#4150
Closed
cdunbar13 wants to merge 2 commits into
Closed
Conversation
mr0re1
reviewed
May 20, 2025
mr0re1
previously approved these changes
May 20, 2025
mr0re1
reviewed
May 22, 2025
mr0re1
reviewed
May 22, 2025
mr0re1
reviewed
Jul 11, 2025
|
|
||
| return placements | ||
|
|
||
| def calculate_hosts_per_topo(gpu_topology: str, machine_type: NSDict) -> int: |
Collaborator
There was a problem hiding this comment.
machine_type is MachineType , not an NSDict, please make sure that mypy is happy)
mr0re1
reviewed
Jul 11, 2025
| top_split = [int(x) for x in str(gpu_topology).split("x")] | ||
| except Exception as e: | ||
| log.error("Incorrectly formatted accelerator topology") | ||
| return 0 |
Collaborator
There was a problem hiding this comment.
Returning magic number is a bad taste. Let's raise an exception instead.
mr0re1
reviewed
Jul 11, 2025
| # Look for gpu topology first | ||
| if gpu_topology is not None: | ||
| hosts_per_topo = calculate_hosts_per_topo(gpu_topology, machine_type) | ||
| if hosts_per_topo > 0: |
Collaborator
There was a problem hiding this comment.
Catch exception and log instead of using magic comparison.
mr0re1
reviewed
Jul 11, 2025
|
|
||
| return placements | ||
|
|
||
| def calculate_hosts_per_topo(gpu_topology: str, machine_type: NSDict) -> int: |
Collaborator
There was a problem hiding this comment.
It would be nice to add unit tests
mr0re1
reviewed
Jul 11, 2025
| try: | ||
| top_split = [int(x) for x in str(gpu_topology).split("x")] | ||
| except Exception as e: | ||
| log.error("Incorrectly formatted accelerator topology") |
Collaborator
There was a problem hiding this comment.
All errors / exceptions should contain the accelerator_topology value to be useful.
mr0re1
reviewed
Jul 11, 2025
| def calculate_hosts_per_topo(gpu_topology: str, machine_type: NSDict) -> int: | ||
| # Calculate total number of hosts per topology (Assumes format: '1x72') | ||
| try: | ||
| top_split = [int(x) for x in str(gpu_topology).split("x")] |
Collaborator
There was a problem hiding this comment.
gpu_topology is already str, why cast?
mr0re1
reviewed
Jul 11, 2025
|
|
||
| return placements | ||
|
|
||
| def calculate_hosts_per_topo(gpu_topology: str, machine_type: NSDict) -> int: |
Collaborator
There was a problem hiding this comment.
nit. rename gpu_topology -> topology OR accelerator_topology OR topo
mr0re1
reviewed
Jul 11, 2025
| log.error("Incorrectly formatted accelerator topology") | ||
| return 0 | ||
|
|
||
| gpus_per_machine = machine_type.accelerators[0].count |
Collaborator
There was a problem hiding this comment.
This will throw if len(machine_type.accelerators) == 0
mr0re1
reviewed
Jul 11, 2025
| log.error("Incorrectly formatted accelerator topology") | ||
| elif gpus_per_machine <= 0: | ||
| log.error("Cannot use accelerator topology with machine type that has no accelerators") | ||
| elif top_split[1] % machine_type.accelerators[0].count != 0: |
Collaborator
There was a problem hiding this comment.
s/machine_type.accelerators[0].count/gpus_per_machine/
mr0re1
reviewed
Jul 11, 2025
| elif top_split[1] % machine_type.accelerators[0].count != 0: | ||
| log.error("GPU count per node must be a factor of the gpu topology") | ||
| else: | ||
| return (top_split[0] * top_split[1]) // gpus_per_machine |
Collaborator
There was a problem hiding this comment.
Check that top_split[0] > 0, otherwise we up for surprise.
mr0re1
reviewed
Jul 11, 2025
| else: | ||
| return (top_split[0] * top_split[1]) // gpus_per_machine | ||
|
|
||
| return 0 |
mr0re1
reviewed
Jul 11, 2025
|
|
||
|
|
||
| def create_placement_request(pg_name: str, region: str, max_distance: Optional[int]): | ||
| def create_placement_request(pg_name: str, region: str, max_distance: Optional[int], gpu_topology: Optional[str]): |
Collaborator
There was a problem hiding this comment.
nit. rename gpu_topology -> accelerator_topology where applicable, to make codebase less confusing and more "searchable".
mr0re1
reviewed
Jul 11, 2025
| nodeset = self.node_nodeset(node_name) | ||
| return parse_self_link(nodeset.subnetwork).region | ||
|
|
||
| def nodeset_gpu_topology(self, nodeset_name: str) -> str: |
Contributor
|
@cdunbar13 I believe this is no longer needed, hence closing this. Please feel free to re-open if it is still needed. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
In anticipation of new machine_types, this PR adds the accelerator_topology field to the slurm controller. It eventually translates to gpuTopology for the placement policy for the slurm cluster.
This can be a template for a similar field for the TPU version of the nodeset and can share the same variable until it hits
resume.py.This will be tested soon, do not merge.