enh: Select platform optimizations at runtime #995

hershys-aws · 2025-09-03T19:05:24Z

Description of changes:
Replace compile-time AWS platform detection with runtime EFA device detection using hwloc. This allows a single binary to work on both AWS and non-AWS environments, automatically enabling optimizations when EFA hardware is present. Removes autotools platform checks and always builds AWS platform code.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

bwbarrett

I stopped after nccl_ofi_platform.cpp, because there are some fairly significant architectural approaches to the problem.

The commit message needs a much more detailed description of the change and why you're making it.

src/nccl_ofi_platform.cpp

hershys-aws · 2025-09-12T17:10:57Z

Split changes out into 2 PR's a refactor and the actual selection logic (this PR). Waiting on other PR to be merged but just posting this for review. Please review the latest commit ONLY as that is what is relevant to this PR.

src/Makefile.am

src/nccl_ofi_net.cpp

src/nccl_ofi_topo.cpp

src/platform-aws.cpp

tests/unit/aws_platform_mapper.cpp

src/platform-aws.cpp

configure.ac

src/nccl_ofi_net.cpp

src/Makefile.am

include/platform-aws.h

configure.ac

include/nccl_ofi_param.h

include/nccl_ofi_platform.h

src/nccl_ofi_platform.cpp

src/nccl_ofi_topo.cpp

src/platform-aws.cpp

src/tuner/nccl_ofi_tuner.cpp

src/nccl_ofi_topo.cpp

src/platform-aws.cpp

include/nccl_ofi_param.h

src/platform-aws.cpp

hershys-aws · 2025-09-29T23:38:28Z

bot:aws:retest

bwbarrett

Didn't make it through the full PR, will try to finish today.

configure.ac

include/nccl_ofi_param.h

include/platform-aws.h

include/nccl_ofi_platform.h

src/nccl_ofi_platform.cpp

src/nccl_ofi_topo.cpp

src/platform-aws.cpp

tests/unit/aws_platform_mapper.cpp

bwbarrett · 2025-10-01T19:02:50Z

tests/unit/platform_manager.cpp

 public:
 	const char* get_name() const override { return "TestPlatform"; }
-	int get_priority() override { return 10; }
+	int get_priority() override { return 200; }


This does not seem like a good design pattern.

I had to keep this as part of the priority value approach...not sure how else to modify/change this?

Replace compile-time AWS platform detection with runtime EFA device detection using hwloc. This allows a single binary to work on both AWS and non-AWS environments, automatically enabling optimizations when EFA hardware is present. Removes autotools platform checks and always builds AWS platform code. Signed-off-by: Hershel Shah <[email protected]>

bwbarrett · 2025-10-07T02:49:00Z

configure.ac

-echo "* Platform-specific optimizations: ${NCCL_OFI_PLATFORM}"
+AS_IF([test "${NCCL_OFI_PLATFORM}" = "none"],
+      [echo "* Platform Optimizations: DISABLED"],
+      [echo "* Platform Optimizations: ${NCCL_OFI_PLATFORM}"])


This isn't quite what I asked for. "DISABLED" is really wrong, since default will still be built. Just build up a list of the enabled platforms (including the default plugin, of course).

bwbarrett · 2025-10-07T02:50:25Z

include/nccl_ofi_param.h

+/*
+ * Override platform selection. Valid options: "AWS", "Default", or empty string for auto-detection.
+ */
+OFI_NCCL_PARAM(std::string, platform_override, "PLATFORM_OVERRIDE", "");


Why does "override" make sense? It doesn't match all the other option we have (like OFI_NCCL_PROGRESS_MODEL above). This should just be OFI_NCCL_PLATFORM.

bwbarrett · 2025-10-07T02:59:03Z

include/nccl_ofi_topo.h

 */
 int nccl_ofi_topo_write_nccl_topology(nccl_ofi_topo_t *topo, FILE *file);

+class TopologyManager {


I strongly object to half-refactoring this code. There's also no need for a topology manager object.

@bwbarrett The reasoning for this was that this file is going to be refactored as part of the c++ refactor in the near future. As a result I figured might be a good time to get started on it. I can revert it and just have a static unique pointer which would get the global topology as an alternative. Given the C++ refactor should I stick with this approach or use the static pointer alternative?

hershys-aws self-assigned this Sep 3, 2025

hershys-aws requested review from bwbarrett and a team as code owners September 3, 2025 19:05

hershys-aws force-pushed the always-enable-aws branch from ff81802 to 83b2db1 Compare September 3, 2025 19:08

bwbarrett requested changes Sep 3, 2025

View reviewed changes

src/nccl_ofi_platform.cpp Outdated Show resolved Hide resolved

src/nccl_ofi_platform.cpp Outdated Show resolved Hide resolved

src/nccl_ofi_platform.cpp Outdated Show resolved Hide resolved

hershys-aws mentioned this pull request Sep 5, 2025

refactor: Move platform optimization code to c++ #996

Merged

hershys-aws force-pushed the always-enable-aws branch from 83b2db1 to 00cb220 Compare September 12, 2025 17:05

hershys-aws changed the title ~~enh: Optimize for AWS platform at runtime~~ enh: Select platform optimizations at runtime Sep 12, 2025

hershys-aws force-pushed the always-enable-aws branch from 00cb220 to 22bfc33 Compare September 12, 2025 19:00

AvivBenchorin reviewed Sep 15, 2025

View reviewed changes

hershys-aws force-pushed the always-enable-aws branch from 22bfc33 to c0e66a1 Compare September 18, 2025 20:44

hershys-aws requested review from AvivBenchorin and bwbarrett September 18, 2025 20:46

rauteric reviewed Sep 18, 2025

View reviewed changes

configure.ac Outdated Show resolved Hide resolved

src/nccl_ofi_net.cpp Outdated Show resolved Hide resolved

src/Makefile.am Outdated Show resolved Hide resolved

include/platform-aws.h Outdated Show resolved Hide resolved

bwbarrett requested changes Sep 19, 2025

View reviewed changes

hershys-aws force-pushed the always-enable-aws branch 3 times, most recently from b13016c to 540e588 Compare September 24, 2025 20:51

hershys-aws requested review from rauteric and bwbarrett September 24, 2025 21:02

hershys-aws force-pushed the always-enable-aws branch 2 times, most recently from 41d7b69 to 63b0161 Compare September 25, 2025 17:25

rauteric reviewed Sep 25, 2025

View reviewed changes

src/tuner/nccl_ofi_tuner.cpp Outdated Show resolved Hide resolved

src/nccl_ofi_topo.cpp Outdated Show resolved Hide resolved

src/platform-aws.cpp Outdated Show resolved Hide resolved

hershys-aws force-pushed the always-enable-aws branch 2 times, most recently from 9c31d48 to 628554e Compare September 25, 2025 21:06

AvivBenchorin reviewed Sep 25, 2025

View reviewed changes

src/platform-aws.cpp Outdated Show resolved Hide resolved

include/nccl_ofi_param.h Outdated Show resolved Hide resolved

src/platform-aws.cpp Outdated Show resolved Hide resolved

hershys-aws force-pushed the always-enable-aws branch from 628554e to 5e06486 Compare September 25, 2025 23:21

hershys-aws requested review from AvivBenchorin and rauteric September 25, 2025 23:22

rauteric previously approved these changes Sep 29, 2025

View reviewed changes

hershys-aws force-pushed the always-enable-aws branch from 5e06486 to 59f43f0 Compare September 29, 2025 20:21

bwbarrett reviewed Sep 30, 2025

View reviewed changes

bwbarrett requested changes Oct 1, 2025

View reviewed changes

hershys-aws dismissed rauteric’s stale review via e842618 October 6, 2025 22:17

hershys-aws force-pushed the always-enable-aws branch 2 times, most recently from e842618 to 4124747 Compare October 6, 2025 22:24

hershys-aws force-pushed the always-enable-aws branch from 4124747 to 62d3961 Compare October 6, 2025 22:31

bwbarrett reviewed Oct 7, 2025

View reviewed changes

enh: Select platform optimizations at runtime #995

Are you sure you want to change the base?

enh: Select platform optimizations at runtime #995

Conversation

hershys-aws commented Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bwbarrett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hershys-aws commented Sep 12, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hershys-aws commented Sep 29, 2025

Uh oh!

bwbarrett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bwbarrett Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

hershys-aws Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

bwbarrett Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

bwbarrett Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

bwbarrett Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

hershys-aws Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hershys-aws commented Sep 3, 2025 •

edited

Loading