add more logging fields that can be set in construction time by zhaojuanmao · Pull Request #51260 · pytorch/pytorch

zhaojuanmao · 2021-01-28T01:36:35Z

Stack from ghstack:

log newly added construction and runtime stats at randomly selected iterations #51394 log newly added construction and runtime stats at randomly selected iterations
add stats that can only be collected at runtime #51386 add stats that can only be collected at runtime
add more logging fields that can be set in construction time #51260 add more logging fields that can be set in construction time

add more logging fields to DDPLoggingData, including param stats, bucket stats, environment variables, nccl version, data type

Differential Revision: D26118245

add more logging fields to DDPLoggingData, including param stats, bucket stats, environment variables, nccl version, data type Differential Revision: [D26118245](https://our.internmc.facebook.com/intern/diff/D26118245/) [ghstack-poisoned]

facebook-github-bot · 2021-01-28T01:36:40Z

💊 CI failures summary and remediations

As of commit 0b04c41 (more details on the Dr. CI page):

1/1 failures possibly* introduced in this PR
- 1/1 non-CircleCI failure(s)

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

add more logging fields to DDPLoggingData, including param stats, bucket stats, environment variables, nccl version, data type Differential Revision: [D26118245](https://our.internmc.facebook.com/intern/diff/D26118245/) ghstack-source-id: 120541380 Pull Request resolved: #51260

add more logging fields to DDPLoggingData, including param stats, bucket stats, environment variables, nccl version, data type Differential Revision: [D26118245](https://our.internmc.facebook.com/intern/diff/D26118245/) [ghstack-poisoned]

Pull Request resolved: #51260 add more logging fields to DDPLoggingData, including param stats, bucket stats, environment variables, nccl version, data type ghstack-source-id: 120603119 Differential Revision: [D26118245](https://our.internmc.facebook.com/intern/diff/D26118245/)

add more logging fields to DDPLoggingData, including param stats, bucket stats, environment variables, nccl version, data type Differential Revision: [D26118245](https://our.internmc.facebook.com/intern/diff/D26118245/) [ghstack-poisoned]

Pull Request resolved: #51260 add more logging fields to DDPLoggingData, including param stats, bucket stats, environment variables, nccl version, data type ghstack-source-id: 120693261 Differential Revision: [D26118245](https://our.internmc.facebook.com/intern/diff/D26118245/)

add more logging fields to DDPLoggingData, including param stats, bucket stats, environment variables, nccl version, data type Differential Revision: [D26118245](https://our.internmc.facebook.com/intern/diff/D26118245/) [ghstack-poisoned]

Pull Request resolved: #51260 add more logging fields to DDPLoggingData, including param stats, bucket stats, environment variables, nccl version, data type ghstack-source-id: 120805134 Differential Revision: [D26118245](https://our.internmc.facebook.com/intern/diff/D26118245/)

zhaojuanmao · 2021-02-02T18:25:12Z

@rohan-varma thanks for reviewing the diff. I can see your major concern is about logging the env variables that depends on the specific backends.

Here are the reasons why I log them as it is right now:

env variables are logged mainly for better debug ability. but env variable list is huge, as you can see an uncompleted list in "_dump_DDP_relevant_env_vars()" in "torch/nn/parallel/distributed.py", I only cherry picked important ones from the list as a starting point. ==> if we need more env variables logged later on, like "high priority stream" flags and etc, it can be easily added later on.
DDPLoggingData struct is a generic data struct that could be used in different logging APIs, where users may need fixed schema. So I thought giving a fixed set of fields will be easier and flexible to manage.
no matter the backend is gloo or nccl, sometimes the env variables are set statically in users' development environment so that they can switch gloo and nccl easily in their codes. So if the users specify the nccl as backend, some gloo env variables could still get valid values and are logged. If env variables are not set, I checked they are logged as "N/A" as expected.

For now, I think we can either 1) log the small set of env variables as the starting pointing in this diff 2) remove NCCL/Gloo env variables right now, add them back later on once you fleshed out debug ability design and list the variables we really need. I prefer 1) though, as we can start with them and see how useful they are.

How do you think?

add more logging fields to DDPLoggingData, including param stats, bucket stats, environment variables, nccl version, data type Differential Revision: [D26118245](https://our.internmc.facebook.com/intern/diff/D26118245/) [ghstack-poisoned]

Pull Request resolved: #51260 add more logging fields to DDPLoggingData, including param stats, bucket stats, environment variables, nccl version, data type ghstack-source-id: 120885387 Differential Revision: [D26118245](https://our.internmc.facebook.com/intern/diff/D26118245/)

zhaojuanmao · 2021-02-03T00:29:36Z

regarding separating logging codes from reducer core logic, I think we can have a logger that wraps ddp_logging_data pointer and also reducer pointer, then put all logging related methods into logger. Let me try that

add more logging fields to DDPLoggingData, including param stats, bucket stats, environment variables, nccl version, data type Differential Revision: [D26118245](https://our.internmc.facebook.com/intern/diff/D26118245/) [ghstack-poisoned]

Pull Request resolved: #51260 add more logging fields to DDPLoggingData, including param stats, bucket stats, environment variables, nccl version, data type ghstack-source-id: 121021915 Differential Revision: [D26118245](https://our.internmc.facebook.com/intern/diff/D26118245/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D26118245/)!

add more logging fields to DDPLoggingData, including param stats, bucket stats, environment variables, nccl version, data type Differential Revision: [D26118245](https://our.internmc.facebook.com/intern/diff/D26118245/) [ghstack-poisoned]

Pull Request resolved: #51260 add more logging fields to DDPLoggingData, including param stats, bucket stats, environment variables, nccl version, data type ghstack-source-id: 121022069 Differential Revision: [D26118245](https://our.internmc.facebook.com/intern/diff/D26118245/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D26118245/)!

add more logging fields to DDPLoggingData, including param stats, bucket stats, environment variables, nccl version, data type Differential Revision: [D26118245](https://our.internmc.facebook.com/intern/diff/D26118245/) [ghstack-poisoned]

Pull Request resolved: #51260 add more logging fields to DDPLoggingData, including param stats, bucket stats, environment variables, nccl version, data type ghstack-source-id: 121045907 Differential Revision: [D26118245](https://our.internmc.facebook.com/intern/diff/D26118245/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D26118245/)!

rohan-varma · 2021-02-04T21:08:37Z

@rohan-varma thanks for reviewing the diff. I can see your major concern is about logging the env variables that depends on the specific backends.

Here are the reasons why I log them as it is right now:

env variables are logged mainly for better debug ability. but env variable list is huge, as you can see an uncompleted list in "_dump_DDP_relevant_env_vars()" in "torch/nn/parallel/distributed.py", I only cherry picked important ones from the list as a starting point. ==> if we need more env variables logged later on, like "high priority stream" flags and etc, it can be easily added later on.

DDPLoggingData struct is a generic data struct that could be used in different logging APIs, where users may need fixed schema. So I thought giving a fixed set of fields will be easier and flexible to manage.

no matter the backend is gloo or nccl, sometimes the env variables are set statically in users' development environment so that they can switch gloo and nccl easily in their codes. So if the users specify the nccl as backend, some gloo env variables could still get valid values and are logged. If env variables are not set, I checked they are logged as "N/A" as expected.

For now, I think we can either 1) log the small set of env variables as the starting pointing in this diff 2) remove NCCL/Gloo env variables right now, add them back later on once you fleshed out debug ability design and list the variables we really need. I prefer 1) though, as we can start with them and see how useful they are.

How do you think?

Sure, (1) sounds good to me for now and we can refactor later as needed.

rohan-varma

LGTM, although I'm sort of confused about a line in the test, please double check that, and some other small comments.

add more logging fields to DDPLoggingData, including param stats, bucket stats, environment variables, nccl version, data type Differential Revision: [D26118245](https://our.internmc.facebook.com/intern/diff/D26118245/) [ghstack-poisoned]

Pull Request resolved: #51260 add more logging fields to DDPLoggingData, including param stats, bucket stats, environment variables, nccl version, data type ghstack-source-id: 121102207 Differential Revision: [D26118245](https://our.internmc.facebook.com/intern/diff/D26118245/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D26118245/)!

add more logging fields to DDPLoggingData, including param stats, bucket stats, environment variables, nccl version, data type Differential Revision: [D26118245](https://our.internmc.facebook.com/intern/diff/D26118245/) [ghstack-poisoned]

Pull Request resolved: #51260 add more logging fields to DDPLoggingData, including param stats, bucket stats, environment variables, nccl version, data type ghstack-source-id: 121260224 Differential Revision: [D26118245](https://our.internmc.facebook.com/intern/diff/D26118245/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D26118245/)!

facebook-github-bot · 2021-02-10T07:13:43Z

This pull request has been merged in 18e0a61.

…#51260) Summary: Pull Request resolved: pytorch#51260 add more logging fields to DDPLoggingData, including param stats, bucket stats, environment variables, nccl version, data type ghstack-source-id: 121260224 Test Plan: unit tests Reviewed By: rohan-varma Differential Revision: D26118245 fbshipit-source-id: ba48b7a11340bda1f5f3b24c8603545d346361e9

zhaojuanmao requested review from mingzhe09088, mrshenli, pritamdamania87 and rohan-varma as code owners January 28, 2021 01:36

facebook-github-bot added cla signed oncall: distributed Add this issue/PR to distributed oncall triage queue labels Jan 28, 2021

zhaojuanmao mentioned this pull request Jan 28, 2021

add a c++ interface in processGroup to get its backend name #51066

Closed

zhaojuanmao mentioned this pull request Jan 29, 2021

Create PyTorch DDP logging APIs for applications to use #50637

Closed

This was referenced Jan 29, 2021

add stats that can only be collected at runtime #51386

Closed

log newly added construction and runtime stats at randomly selected iterations #51394

Closed

rohan-varma reviewed Feb 2, 2021

View reviewed changes

rohan-varma reviewed Feb 4, 2021

View reviewed changes

Comment thread torch/lib/c10d/logger.cpp Outdated

rohan-varma reviewed Feb 4, 2021

View reviewed changes

Comment thread torch/nn/parallel/distributed.py

rohan-varma approved these changes Feb 4, 2021

View reviewed changes

Comment thread torch/testing/_internal/distributed/distributed_test.py Outdated

rohan-varma reviewed Feb 4, 2021

View reviewed changes

Comment thread torch/lib/c10d/logger.cpp

facebook-github-bot closed this in 18e0a61 Feb 10, 2021

facebook-github-bot added the Merged label Feb 10, 2021

facebook-github-bot deleted the gh/zhaojuanmao/58/head branch February 13, 2021 15:18

Conversation

zhaojuanmao commented Jan 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Jan 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zhaojuanmao commented Feb 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhaojuanmao commented Feb 3, 2021

Uh oh!

rohan-varma commented Feb 4, 2021

Uh oh!

Uh oh!

Uh oh!

rohan-varma left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

facebook-github-bot commented Feb 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zhaojuanmao commented Jan 28, 2021 •

edited

Loading

facebook-github-bot commented Jan 28, 2021 •

edited

Loading

zhaojuanmao commented Feb 2, 2021 •

edited

Loading