Skip to content

reference tensor proto of tensorflow #1729

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

Merged
merged 9 commits into from
Feb 14, 2020

Conversation

QiJune
Copy link
Collaborator

@QiJune QiJune commented Feb 13, 2020

Fix #1728


$(GO_PB_FILE): $(EDL_PROTO_FILE)
go_pb:
Copy link
Collaborator

@wangkuiyi wangkuiyi Feb 14, 2020

Choose a reason for hiding this comment

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

This target requires at least one dependent ${PROTO_PATH}/elasticdl.proto

go_pb: elasticdl/proto/elasticdl.proto
    ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

CXX = g++
CAPI_DIR = elasticdl/pkg/kernel/capi

all: python_pb $(GO_PB_FILE) kernel
all: python_pb go_pb kernel

python_pb:
Copy link
Collaborator

@wangkuiyi wangkuiyi Feb 14, 2020

Choose a reason for hiding this comment

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

This target requires at least ${PROTO_PATH}/elasticdl.proto as its dependent.

python_pb: elasticdl/proto/elasticdl.proto
    ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done


EDL_PROTO_FILE = elasticdl/proto/elasticdl.proto

PROTO_PATH=elasticdl/proto
Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer we don't define this variable, as its naming doesn't add much information. Instead, we could literally use elasticdl/proto in the following rules.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@@ -49,11 +49,11 @@ RUN /install-protobuf.bash && rm /install-protobuf.bash

# Install elasticdl.org/elasticdl Go package
ENV ELASTICDLPATH $GOPATH/src/elasticdl.org/elasticdl
ENV TF_PATH /tmp/tensorflow
RUN cd /tmp && git clone --depth=1 https://github.com/tensorflow/tensorflow.git
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding this line would significantly slow down the Docker build speed. @QiJune please follow up #1731 with @typhoonzero to use Docker build cache as much as possible.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer to fix #1731 before merging this PR; otherwise, the merge of this PR would significantly slow down the CI until we fix #1731.

Copy link
Collaborator Author

@QiJune QiJune Feb 14, 2020

Choose a reason for hiding this comment

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

@wangkuiyi I compare two CI time of two PRs, PR 1726 and current PR

  • PR 1726: 6min 15s + 17min 34s = 23min 49s
  • current PR: 6min 6s + 16min 49s = 22min 55s

22min 55s is smaller than 23min 49s. It seems that downloading tensorflow will not influence the CI speed.

I usegit clone --depth=1, so the downloading speed will be fast.

@QiJune QiJune merged commit c8c76f5 into sql-machine-learning:develop Feb 14, 2020
@QiJune QiJune deleted the use_tf_proto branch August 7, 2020 02:39
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.

Reuse TensorFlow protobuf messages
2 participants