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

[Dist] enable USE_EPOLL in default #4167

Merged
merged 9 commits into from
Jun 27, 2022

Conversation

Rhett-Ying
Copy link
Collaborator

Description

Checklist

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [$CATEGORY] (such as [NN], [Model], [Doc], [Feature]])
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage
  • Code is well-documented
  • To the best of my knowledge, examples are either not affected by this change,
    or have been fixed to be compatible with this change
  • Related issue is referred in this PR
  • If the PR is for a new model/paper, I've updated the example index here.

Changes

@dgl-bot
Copy link
Collaborator

dgl-bot commented Jun 25, 2022

To trigger regression tests:

  • @dgl-bot run [instance-type] [which tests] [compare-with-branch];
    For example: @dgl-bot run g4dn.4xlarge all dmlc/master or @dgl-bot run c5.9xlarge kernel,api dmlc/master

@dgl-bot
Copy link
Collaborator

dgl-bot commented Jun 25, 2022

Commit ID: f2dfa3dc15d2376149bb6618f6ba74b50d2c9284

Build ID: 1

Status: ❌ CI test failed in Stage [Lint Check].

Report path: link

Full logs path: link

@dgl-bot
Copy link
Collaborator

dgl-bot commented Jun 25, 2022

Commit ID: 79cec6f

Build ID: 2

Status: ❌ CI test failed in Stage [CPU Build].

Report path: link

Full logs path: link

@dgl-bot
Copy link
Collaborator

dgl-bot commented Jun 25, 2022

Commit ID: c923832

Build ID: 3

Status: ❌ CI test failed in Stage [GPU Build].

Report path: link

Full logs path: link

@dgl-bot
Copy link
Collaborator

dgl-bot commented Jun 25, 2022

Commit ID: ec1bd1f

Build ID: 4

Status: ❌ CI test failed in Stage [CPU Build].

Report path: link

Full logs path: link

@dgl-bot
Copy link
Collaborator

dgl-bot commented Jun 25, 2022

Commit ID: 0aa5c25

Build ID: 5

Status: ❌ CI test failed in Stage [CPU Build].

Report path: link

Full logs path: link

@dgl-bot
Copy link
Collaborator

dgl-bot commented Jun 25, 2022

Commit ID: fc2072f

Build ID: 6

Status: ❌ CI test failed in Stage [CPU Build].

Report path: link

Full logs path: link

@dgl-bot
Copy link
Collaborator

dgl-bot commented Jun 25, 2022

Commit ID: 388a468

Build ID: 7

Status: ❌ CI test failed in Stage [CPU Build].

Report path: link

Full logs path: link

@dgl-bot
Copy link
Collaborator

dgl-bot commented Jun 25, 2022

Commit ID: 1f077d5

Build ID: 8

Status: ✅ CI test succeeded

Report path: link

Full logs path: link

@@ -122,7 +122,8 @@ if(USE_AVX)
endif(USE_LIBXSMM)
endif(USE_AVX)

if (USE_EPOLL)
if ((NOT MSVC) AND USE_EPOLL)
INCLUDE(CheckIncludeFile)
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if INCLUDE(CheckIncludeFile) is not added, error is Unknown CMake command 'check_include_file'.

@Rhett-Ying Rhett-Ying merged commit 9d42531 into dmlc:master Jun 27, 2022
@Rhett-Ying Rhett-Ying deleted the distEnableUSE_EPOLL branch June 27, 2022 01:12
@engaineer
Copy link

engaineer commented Jun 28, 2022

@Rhett-Ying Changing to use epoll by default breaks macOS build on either x86_64 or arm64 arch.

dgl_option(USE_EPOLL "Build with epoll for socket communicator" ON)

Temporary measure suggetions:

  • Update build docs to add compiler flag -DUSE_EPOLL=OFF
  • Update CMakeLists.txt to detect STREQUAL "Darwin" and revert to setting to USE_EPOLL=OFF and print a warning.

It's a great idea to use epoll to manage the socketpool but not cross-platform friendly. Is PR open to replace with something else like libuv, or is there another plan?

@Rhett-Ying
Copy link
Collaborator Author

Thanks for reporting this. I will fix it.

@Rhett-Ying
Copy link
Collaborator Author

@engaineer pls try with the latest master branch...

@engaineer
Copy link

@engaineer pls try with the latest master branch...

Clean build on macOS 12.4 arch arm64 for master with HEAD 1518861.

Thanks for being awesome and providing a prompt fix and refinement.

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.

None yet

4 participants