add support to metrics calculation #195
Conversation
TaekyungHeo
requested review from
kingchc,
louisfeng,
sunghlin,
shengbao-zheng and
briancoutinho
as code owners
facebook-github-bot
added
the
CLA Signed
There was a problem hiding this comment.
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
| @@ -622,6 +622,9 @@ def barrier(self, collectiveArgs, name="dummy", retFlag=False): | |||
|
|
|||
| if retFlag: | |||
| return retObj | |||
|
|
|||
| def barrier_all_ranks(self): | |||
| dist.barrier(device_ids=[self.get_device().index] if dist.get_backend() == "nccl" else None) | |||
There was a problem hiding this comment.
question, why is this changed from the simplle barrie() before?
There was a problem hiding this comment.
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]] = { |
There was a problem hiding this comment.
nit maybe call these functions?
There was a problem hiding this comment.
_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_')] |
There was a problem hiding this comment.
nit could be startswith('nccl') for older nccl kernel names that did not start with Device
There was a problem hiding this comment.
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_')] |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
we could events processing that failed and print at bottom
| 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]) |
There was a problem hiding this comment.
nit this will work
total_data_size = sum(_calculate_event_data_size(evt) * _get_event_busbw_factor(evt) for evt in nccl_events)
|
|
||
| return total_data_size / (end_time_point - begin_time_point - total_idle_time) / 1e3 | ||
|
|
||
| def pick_iter_e2e_time_(trace_data, tl): |
There was a problem hiding this comment.
please add some doc on this function, not clear what it is doing, and what is tl
|
|
||
| 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]) |
There was a problem hiding this comment.
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') |
There was a problem hiding this comment.
You can consider using tabulate library since you are considering adding dependency.
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
|
@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:
|
|
|
PR196 seems not include this commit: 57f619e |
|
Hi, @shengfukevin
|
summary
Add support to metrics calculation.
test plan