Skip to content

Conversation

@TaekyungHeo
Copy link
Collaborator

summary

Add support to metrics calculation.

  1. Iteration E2E time;
  2. bandwidth;

test plan

srun -l -N 8 -n 64 --ntasks-per-node=8 --container-name=sanshang_test \
--container-mounts=/labhome/sanshang:/labhome/sanshang,/swgwork/sanshang:/labhome/sanshang/storage \
bash -c "comm_replay --trace-type et --trace-path ${ROOT_PATH}/et_trace_64gpus \
--output-path ${ROOT_PATH}/output \
--enable-profiler --profiler-num-replays-start 4 --profiler-num-replays 5 \
--do-warm-up --reuse-tensors --num-replays 10"

image
image

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 10, 2025
Copy link
Contributor

@briancoutinho briancoutinho left a comment

Choose a reason for hiding this comment

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

Left some suggestions. @shengfukevin also saw black formatting issues, which we would need to fix to pass linter, Sheng is there someway we can share the lint suggestions? Let's do that after next iteration of the PR

return retObj

def barrier_all_ranks(self):
dist.barrier(device_ids=[self.get_device().index] if dist.get_backend() == "nccl" else None)
Copy link
Contributor

Choose a reason for hiding this comment

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

question, why is this changed from the simplle barrie() before?

Copy link
Contributor

@sanshang-nv sanshang-nv Jan 23, 2025

Choose a reason for hiding this comment

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

This is new added util function, not exist before. To make all dist API call be encapsulated in backend module.

Another point is, if not specify device id, there is warning in log to use GPU0 for barrier. So we should take here as a reference.

}

# refer to: https://github.com/NVIDIA/nccl-tests/blob/master/doc/PERFORMANCE.md
_busbw_correction_factors_tbl: Dict[str, Callable[[int], float]] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit maybe call these functions?

Copy link
Contributor

Choose a reason for hiding this comment

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

_busbw_correction_factors_fns

return correction_factor_func(group_size)

def calculate_bw_(trace_data):
nccl_events = [i for i in trace_data['traceEvents'] if i.get('cat', '') == 'kernel' and i['name'].startswith('ncclDevKernel_')]
Copy link
Contributor

Choose a reason for hiding this comment

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

nit could be startswith('nccl') for older nccl kernel names that did not start with Device

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean, older nccl kernel in latest nccl verion? Is there some examples? I don't assume we need to be compatible with old nccl version. But if new nccl version has older nccl kernels, we need to consider refine this.

return correction_factor_func(group_size)

def calculate_bw_(trace_data):
nccl_events = [i for i in trace_data['traceEvents'] if i.get('cat', '') == 'kernel' and i['name'].startswith('ncclDevKernel_')]
Copy link
Contributor

Choose a reason for hiding this comment

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

to save memory consider using generator
nccl_events = (e for e in trace_data['traceEvents'] if e.get('cat', '') == 'kernel' and e['name'].startswith('ncclDevKernel_'))

evt['args']['busbw (GB/sec)'] = busbw
evt['args']['busbw_factor'] = busbw_factor
except ValueError as e:
logger.error('Error processing event: %s', e)
Copy link
Contributor

Choose a reason for hiding this comment

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

we could events processing that failed and print at bottom

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

if not len(nccl_events):
return 0

total_data_size = sum([_calculate_event_data_size(evt) * _get_event_busbw_factor(evt) for evt in nccl_events])
Copy link
Contributor

Choose a reason for hiding this comment

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

nit this will work
total_data_size = sum(_calculate_event_data_size(evt) * _get_event_busbw_factor(evt) for evt in nccl_events)

Copy link
Contributor

Choose a reason for hiding this comment

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

Done


return total_data_size / (end_time_point - begin_time_point - total_idle_time) / 1e3

def pick_iter_e2e_time_(trace_data, tl):
Copy link
Contributor

Choose a reason for hiding this comment

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

please add some doc on this function, not clear what it is doing, and what is tl

Copy link
Contributor

Choose a reason for hiding this comment

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

done


total_data_size = sum([_calculate_event_data_size(evt) * _get_event_busbw_factor(evt) for evt in nccl_events])

time_range_tree = IntervalTree([Interval(evt['ts'], evt['ts'] + evt['dur']) for evt in nccl_events])
Copy link
Contributor

Choose a reason for hiding this comment

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

Today i learned Interval tree class nice :)


# dump summary report
with open(os.path.join(report_dir, 'profiler_trace_summary_report.txt'), 'w', encoding='utf-8') as f:
f.write(f'avg. E2ETime of iters among all ranks: {sum(iter_e2e_time) / len(iter_e2e_time) / 1e3 :.3f} ms\n')
Copy link
Contributor

Choose a reason for hiding this comment

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

You can consider using tabulate library since you are considering adding dependency.

facebook-github-bot pushed a commit that referenced this pull request Jan 23, 2025
Summary:
Add support to metrics calculation.

1. Iteration E2E time
2. bandwidth

This is the copy of #195 for importing it into Meta.


Differential Revision: D68038126

Pulled By: shengfukevin
@shengfukevin
Copy link
Contributor

@briancoutinho @sanshang-nv @TaekyungHeo , I copied this PR and imported to Meta, this is the new diff #196. Please update on PR 196.

I have two issues that need fix:

  1. When profiler is on, this PR will create two profilers, one for meta internal, one for OSS. I fixed that in PR196.
  2. Meta internal need to support uploading report to a Meta internal repository when --output is in the options. I am still working on it.

@sanshang-nv
Copy link
Contributor

When profiler is on, this PR will create two profilers, one for meta internal, one for OSS. I fixed that in PR196.
@shengfukevin what do you mean 2 profilers? When meta internal works, I will skip to not make public profiler work.

@sanshang-nv
Copy link
Contributor

PR196 seems not include this commit: 57f619e

@sanshang-nv
Copy link
Contributor

Hi, @shengfukevin
Has updated to:

  1. solve your 1st issue of 2 profilers when running inside Meta
  2. resolve some comments from Brian.

@shengfukevin
Copy link
Contributor

@sanshang-nv, after took another look, MetaProfiler seems an overkill to provide a Meta internal profiler. The difference between MetaProfiler and public profiler are 1. the supported activities 2. the call back function when trace is ready. Inside Meta, we already have fb directory, it seems redundant to have vendor_internal directory.

When you create the profiler, can you just pass in different activities and callback function like the following:

try:
from param_bench.et_replay.fb.internals import (
get_fb_profiler_activities,
get_fb_profiler_trace_handler,
)

has_internal_libs = True

except ImportError:
has_internal_libs = False

if has_internal_libs:
activities = get_fb_profiler_activities(device)
trace_handler = get_fb_profiler_trace_handler(rank)
else:
activities = {ProfilerActivity.CPU, ProfilerActivity.CUDA}

    def trace_handler(p):
        import pathlib

        folder_path = os.path.join(output_path, "profiler_trace")
        try:
            pathlib.Path(folder_path).mkdir(parents=True, exist_ok=True)
        except PermissionError:
            logger.error(f"Permission denied to create directory {folder_path}")
        p.export_chrome_trace(os.path.join(folder_path, f"rank-{rank}.json"))

logger.debug("GPU Trace Collection: Enabled")
_torch_profiler = torch.profiler.profile(
    schedule=torch.profiler.schedule(
        wait=1,
        warmup=numWarmupIters,
        active=numIters,
        repeat=1,
    ),
    on_trace_ready=trace_handler,
    activities=activities,
)

Also I think we still need warmup-iter so user can control how many iterations they want to run the warmup.

Thanks
Sheng

@sanshang-nv
Copy link
Contributor

sanshang-nv commented Feb 7, 2025

Inside Meta, we already have fb directory, it seems redundant to have vendor_internal directory.

I don't think so. vendor_internal is not only for Meta, but also for NV and other companies, to decouple vendor's internal features.

param_bench.et_replay.fb.internals

I don't find fb.internals module in param\et_replay directory. Is it Meta private? I can just use it as below, right?

from param_bench.et_replay.fb.internals import (
get_fb_profiler_activities,
get_fb_profiler_trace_handler,
)

Then, we remove the usage to these 3 fb functions, right?

    from param_bench.train.comms.pt.fb.internals import (
        fbInitProfiler,
        fbSampleProfiler,
        fbStartProfiler,
    )

Because this is black box to me, it's hard to decide which implementation is the best from my side.

Also I think we still need warmup-iter so user can control how many iterations they want to run the warmup.

Considering add this option back.

@shengfukevin
Copy link
Contributor

ok, then let's keep vendor_internal. fb.internal got filtered out when export param from meta.

You can call

from param_bench.et_replay.fb.internals import (
get_fb_profiler_activities,
get_fb_profiler_trace_handler,
)

inside vendor_internal/fb_internal.py

Remove fbInitProfiler,
fbSampleProfiler,
fbStartProfiler,

Thanks

facebook-github-bot pushed a commit that referenced this pull request Feb 10, 2025
Summary:
Add support to metrics calculation.

1. Iteration E2E time
2. bandwidth

This is the copy of #195 for importing it into Meta.


Reviewed By: briancoutinho

Differential Revision: D69155629

Pulled By: shengfukevin
facebook-github-bot pushed a commit that referenced this pull request Feb 10, 2025
Summary:
Add support to metrics calculation.

1. Iteration E2E time
2. bandwidth

This is the copy of #195 for importing it into Meta.


Reviewed By: briancoutinho

Differential Revision: D69155629

Pulled By: shengfukevin
facebook-github-bot pushed a commit that referenced this pull request Feb 10, 2025
Summary:
Add support to metrics calculation.

1. Iteration E2E time
2. bandwidth

This is the copy of #195 for importing it into Meta.


Reviewed By: briancoutinho

Differential Revision: D69155629

Pulled By: shengfukevin
facebook-github-bot pushed a commit that referenced this pull request Feb 10, 2025
Summary:
Add support to metrics calculation.

1. Iteration E2E time
2. bandwidth

This is the copy of #195 for importing it into Meta.


Reviewed By: briancoutinho

Differential Revision: D69155629

Pulled By: shengfukevin
facebook-github-bot pushed a commit that referenced this pull request Feb 10, 2025
Summary:
Add support to metrics calculation.

1. Iteration E2E time
2. bandwidth

This is the copy of #195 for importing it into Meta.


Reviewed By: briancoutinho

Differential Revision: D69155629

Pulled By: shengfukevin
facebook-github-bot pushed a commit that referenced this pull request Feb 11, 2025
Summary:
Add support to metrics calculation.

1. Iteration E2E time
2. bandwidth

This is the copy of #195 for importing it into Meta.


Reviewed By: briancoutinho

Differential Revision: D69155629

Pulled By: shengfukevin
facebook-github-bot pushed a commit that referenced this pull request Feb 11, 2025
Summary:
Add support to metrics calculation.

1. Iteration E2E time
2. bandwidth

This is the copy of #195 for importing it into Meta.


Reviewed By: briancoutinho

Differential Revision: D69155629

Pulled By: shengfukevin
facebook-github-bot pushed a commit that referenced this pull request Feb 11, 2025
Summary:
Add support to metrics calculation.

1. Iteration E2E time
2. bandwidth

This is the copy of #195 for importing it into Meta.


Reviewed By: briancoutinho

Differential Revision: D69155629

Pulled By: shengfukevin
facebook-github-bot pushed a commit that referenced this pull request Feb 11, 2025
Summary:
Add support to metrics calculation.

1. Iteration E2E time
2. bandwidth

This is the copy of #195 for importing it into Meta.


Reviewed By: briancoutinho

Differential Revision: D69155629

Pulled By: shengfukevin
facebook-github-bot pushed a commit that referenced this pull request Feb 13, 2025
Summary:
Add support to metrics calculation.

1. Iteration E2E time
2. bandwidth

This is the copy of #195 for importing it into Meta.


Test Plan: /usr/local/fbcode/platform010/bin/mpirun -np 2 buck2 run mode/opt param_bench/et_replay:comm_replay -- --trace-path manifold://pytorch_execution_trace/tree/traces/shengfu/icvr --enable-profiler --output-path work-dir

Reviewed By: briancoutinho

Differential Revision: D69155629

Pulled By: shengfukevin
facebook-github-bot pushed a commit that referenced this pull request Feb 13, 2025
Summary:
Add support to metrics calculation.

1. Iteration E2E time
2. bandwidth

This is the copy of #195 for importing it into Meta.

Pull Request resolved: #197

Test Plan: /usr/local/fbcode/platform010/bin/mpirun -np 2 buck2 run mode/opt param_bench/et_replay:comm_replay -- --trace-path manifold://pytorch_execution_trace/tree/traces/shengfu/icvr --enable-profiler --output-path work-dir

Reviewed By: briancoutinho

Differential Revision: D69155629

Pulled By: shengfukevin

fbshipit-source-id: d3b21731cd2edfd5a8c570421cbd96cde780fa59
@TaekyungHeo TaekyungHeo deleted the sanshang/calc_metrics branch March 15, 2025 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants