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

Add SGD Kernel Go Package #1637

Merged
merged 21 commits into from Jan 17, 2020
Merged

Add SGD Kernel Go Package #1637

merged 21 commits into from Jan 17, 2020

Conversation

@QiJune
Copy link
Collaborator

QiJune commented Jan 15, 2020

The SGD kernel is implemented in C++ and exposed to Go via CGo.

I make a wrapper kernel.go, so other functions in Go could directly call SGD function in Go.

QiJune added 10 commits Jan 14, 2020
@skydoorkai

This comment has been minimized.

Copy link
Collaborator

skydoorkai commented Jan 15, 2020

In Dockerfile, we should copy elasticdl/pkg to somewhere underGOPATH, then go install


package master;
option go_package = "elasticdl/pkg/proto";

enum TensorDtype {

This comment has been minimized.

Copy link
@skydoorkai

skydoorkai Jan 15, 2020

Collaborator

Probably use a separate PR to combine tensor_dtype.proto and elasticdl.proto together.

This comment has been minimized.

Copy link
@QiJune

QiJune Jan 15, 2020

Author Collaborator

Please refer to #1638

@@ -0,0 +1,27 @@
#!/bin/bash

This comment has been minimized.

Copy link
@wangkuiyi

wangkuiyi Jan 16, 2020

Collaborator

Other than crafting our own scripts, could we use https://github.com/bmorcos/pre-commit-hooks-cpp or other pre-made C++ hooks for pre-commit?

This comment has been minimized.

Copy link
@QiJune

QiJune Jan 16, 2020

Author Collaborator

The precommit hook files are just modified from the paddle repo.

I tried this one https://github.com/bmorcos/pre-commit-hooks-cpp, but got the following error:

[INFO] Installing environment for https://github.com/bmorcos/pre-commit-hooks-cpp.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
An unexpected error has occurred: CalledProcessError: Command: ('/Users/qijun/.cache/pre-commit/repo4qxgesqb/py_env-python3.7/bin/python3.7', '/Users/qijun/.cache/pre-commit/repo4qxgesqb/py_env-python3.7/bin/pip', 'install', '.')
Return code: 1
Expected return code: 0
Output: (none)
Errors:
    ERROR: Directory '.' is not installable. Neither 'setup.py' nor 'pyproject.toml' found.

Check the log at /Users/qijun/.cache/pre-commit/pre-commit.log

We could use our own scripts currently. After debugging this, we could transfer to pre-made C++ hooks.

@@ -0,0 +1,15 @@
extern "C" {

This comment has been minimized.

Copy link
@wangkuiyi

wangkuiyi Jan 16, 2020

Collaborator

In my experience, extern "C" is often used in header files other source files. For example

#ifdef __cplusplus  
extern "C" { 
#endif 
    /* Declarations of this file */
#ifdef __cplusplus 
} 
#endif 

Is this some style requirement to use it in C++ source files?

This comment has been minimized.

Copy link
@QiJune

QiJune Jan 16, 2020

Author Collaborator

Yes, you are right. In we want to use a C header file in a C++ source file, we have to add extern "C" to the C header file. There are two cases:

  • If the C header file is written by myself, I'd better add macros to C header filer.
  • If the C header file is from other places, for example, a Linux system C header file, I have to add extern "C" in C++ source file.

I will fix it.

#define ELASTICDL_KERNEL_API_H_

void SGD(float* grad, float* param, double lr, long long size);
#endif

This comment has been minimized.

Copy link
@wangkuiyi

wangkuiyi Jan 16, 2020

Collaborator

The Google C++ style guide has some requirements on guard https://google.github.io/styleguide/cppguide.html#The__define_Guard

This comment has been minimized.

Copy link
@QiJune

QiJune Jan 16, 2020

Author Collaborator

Done

QiJune added 9 commits Jan 16, 2020
@@ -6,7 +6,7 @@ FROM ${BASE_IMAGE}
COPY elasticdl/docker/bashrc /etc/bash.bashrc
RUN chmod a+rwx /etc/bash.bashrc

RUN apt-get update && apt-get install -y unzip curl git software-properties-common
RUN apt-get update && apt-get install -y unzip curl git software-properties-common libeigen3-dev

This comment has been minimized.

Copy link
@skydoorkai

skydoorkai Jan 17, 2020

Collaborator

For the last line of this file, it will fail as Go is not installed.
We can change it to
RUN make -f elasticdl/Makefile python_pb
for now, and figure out how to install the PS Go version later.

This comment has been minimized.

Copy link
@QiJune

QiJune Jan 17, 2020

Author Collaborator

Done

@QiJune QiJune merged commit 36e99d0 into sql-machine-learning:develop Jan 17, 2020
2 checks passed
2 checks passed
WIP Ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@QiJune QiJune deleted the QiJune:sgd_kernel branch Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.