Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

format: loosely enforce the code style #23

Merged
merged 5 commits into from
Jul 10, 2019
Merged

format: loosely enforce the code style #23

merged 5 commits into from
Jul 10, 2019

Conversation

changlan
Copy link
Contributor

@changlan changlan commented Jun 28, 2019

  • add contribution guidelines
  • reformat all code with clang-format and autopep
  • tweak type casts, conforming to google c++ style

@ymjiang: there should be no functional change, but would be nice to confirm it's working as before.

@changlan changlan requested a review from ymjiang June 28, 2019 18:23
@changlan changlan force-pushed the reformat branch 2 times, most recently from 65fe774 to 72fac70 Compare June 28, 2019 19:24
@byronyi
Copy link
Member

byronyi commented Jun 29, 2019

Install clang-format, and do clang-format -i --style=google to all sources.

@changlan changlan force-pushed the reformat branch 2 times, most recently from 3c00c7c to 27d3381 Compare June 29, 2019 06:48
@ymjiang
Copy link
Member

ymjiang commented Jun 29, 2019

Cannot compile:

In file included from byteps/common/communicator.h:32:0,
                 from byteps/common/global.h:28,
                 from byteps/common/core_loops.cc:21:
byteps/common/core_loops.cc: In function 'void byteps::common::PostNcclCalls(std::shared_ptr<byteps::common::TensorTableEntry>, byteps::common::QueueType)':
byteps/common/core_loops.cc:182:35: error: invalid conversion from 'const void*' to 'void*' [-fpermissive]
           (const void *)p, (out_p + nccl_rank * num_elem_per_gpu * unit_len),
                                   ^
byteps/common/logging.h:64:23: note: in definition of macro 'NCCLCHECK'
     ncclResult_t r = (cmd);                                                 \
                       ^
In file included from byteps/common/common.h:21:0,
                 from byteps/common/core_loops.cc:20:
/usr/local/nccl/include/nccl.h:191:15: note: initializing argument 2 of 'ncclResult_t ncclReduceScatter(const void*, void*, size_t, ncclDataType_t, ncclRedOp_t, ncclComm_t, cudaStream_t)'
 ncclResult_t  ncclReduceScatter(const void* sendbuff, void* recvbuff,
               ^
In file included from byteps/common/communicator.h:32:0,
                 from byteps/common/global.h:28,
                 from byteps/common/core_loops.cc:21:
byteps/common/core_loops.cc:189:41: error: invalid conversion from 'const void*' to 'void*' [-fpermissive]
                            (out_p + len - left_elem * unit_len),
                                         ^
byteps/common/logging.h:64:23: note: in definition of macro 'NCCLCHECK'
     ncclResult_t r = (cmd);                                                 \
                       ^
In file included from byteps/common/common.h:21:0,
                 from byteps/common/core_loops.cc:20:
/usr/local/nccl/include/nccl.h:134:15: note: initializing argument 2 of 'ncclResult_t ncclReduce(const void*, void*, size_t, ncclDataType_t, ncclRedOp_t, int, ncclComm_t, cudaStream_t)'
 ncclResult_t  ncclReduce(const void* sendbuff, void* recvbuff, size_t count, ncclDataType_t datatype,
               ^
byteps/common/core_loops.cc: In function 'void byteps::common::CopyHost2Device(std::shared_ptr<byteps::common::TensorTableEntry>)':
byteps/common/core_loops.cc:522:58: error: reinterpret_cast from type 'const void*' to type 'char*' casts away qualifiers
   auto gpu_addr = reinterpret_cast<char *>(tensor->data()) + offset;
                                                          ^
error: None of TensorFlow, MXNet, PyTorch plugins were built. See errors above.

Besides, I think we should reach a consensus on the code style. @bobzhuyb any preference or suggestion?

@changlan
Copy link
Contributor Author

ps: It's not ready yet, pls wait until CI passes

@changlan changlan force-pushed the reformat branch 3 times, most recently from 42a0983 to 4ecd470 Compare June 29, 2019 08:32
@changlan
Copy link
Contributor Author

@ymjiang Ready now. Please test.

@changlan changlan requested review from ymjiang and removed request for ymjiang July 2, 2019 23:41
@changlan changlan force-pushed the reformat branch 3 times, most recently from de05090 to 4b54979 Compare July 9, 2019 05:21
Copy link
Member

@ymjiang ymjiang left a comment

Choose a reason for hiding this comment

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

Pass tests: (1) TF/MX/Pytorch (2) single machine / distributed. Ready to merge this PR.

@ymjiang ymjiang merged commit 989925b into master Jul 10, 2019
@changlan changlan deleted the reformat branch July 11, 2019 05:02
pleasantrabbit pushed a commit that referenced this pull request Jul 13, 2020
pleasantrabbit pushed a commit that referenced this pull request Nov 3, 2020
* fix add_flags coverage

* remove c++11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants