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 support for running python functions with wheel file #2593

Merged
merged 32 commits into from Sep 24, 2018

Conversation

@srkukarni
Copy link
Contributor

srkukarni commented Sep 17, 2018

Motivation

Currently running python functions require all function logic to be contained within one python file. This pr allows python wheel files to be submitted to be run as functions, thereby greatly increasing flexibility

Modifications

Describe the modifications you've done.

Result

After your change, what will change.

@srkukarni srkukarni requested review from merlimat, sijie and jerrypeng Sep 17, 2018

srkukarni and others added some commits Sep 17, 2018

[build] Fix docker organization parameter
*Motivation*

docker orgnization is missing for building test image. so the build will be failing with `-Pdocker`.

*Changes*

Move the docker organization parameter to root pom file.

if function_details.runtime == Function_pb2.FunctionDetails.Runtime.Value("PYTHON_WHEEL"):
try:
util.install_wheel(str(args.py))

This comment has been minimized.

Copy link
@sijie

sijie Sep 18, 2018

Contributor

I am not familiar with python. but isn't that manipulating the environment, and doesn't it require sudos and such?

This comment has been minimized.

Copy link
@srkukarni

srkukarni Sep 18, 2018

Author Contributor

I've redone that part with installing using virtualenv to minimize impact on the system and needing permissions

@srkukarni srkukarni self-assigned this Sep 18, 2018

@srkukarni srkukarni added this to the 2.2.0-incubating milestone Sep 18, 2018

srkukarni added some commits Sep 18, 2018

@merlimat
Copy link
Contributor

merlimat left a comment

Change LGTM. Just minor comment

@@ -229,6 +229,10 @@ void processArguments() throws Exception {
description = "Path to the main Python file for the function (if the function is written in Python)",
listConverter = StringConverter.class)
protected String pyFile;
@Parameter(
names = "--pywheel",

This comment has been minimized.

Copy link
@merlimat

merlimat Sep 20, 2018

Contributor

Instead of having a new switch, could we reuse the --py and then disambiguate based on the file extension?

@srkukarni

This comment has been minimized.

Copy link
Contributor Author

srkukarni commented Sep 20, 2018

@merlimat I have removed the new runtime. Please take a look again. Thanks!

@ivankelly
Copy link
Contributor

ivankelly left a comment

This needs an integration test.

@@ -74,6 +73,11 @@ def main():
args = parser.parse_args()
function_details = Function_pb2.FunctionDetails()
json_format.Parse(args.function_details, function_details)

if os.path.splitext(str(args.py))[1] == '.whl':
os.system("unzip -d %s -o %s" % (os.path.dirname(str(args.py)), str(args.py)))

This comment has been minimized.

Copy link
@ivankelly

ivankelly Sep 20, 2018

Contributor

unzip is a new dependency.

This comment has been minimized.

Copy link
@ivankelly

ivankelly Sep 20, 2018

Contributor

where is this unzipped to?

This comment has been minimized.

Copy link
@srkukarni

srkukarni Sep 20, 2018

Author Contributor

it is unzipped into the same tmp directory the .whl file is copied to. The -d option controls that.

This comment has been minimized.

Copy link
@srkukarni

srkukarni Sep 20, 2018

Author Contributor

unzip is indeed a new dependency.

This comment has been minimized.

Copy link
@merlimat

merlimat Sep 20, 2018

Contributor

unzip might not be always installed (especially in Docker images where everything is stripped to bone).
Python has a native way to deal with zip files, without invoking the CLI command: https://docs.python.org/3/library/zipfile.html#zipfile.ZipFile.extract

This comment has been minimized.

Copy link
@srkukarni

srkukarni Sep 20, 2018

Author Contributor

I have removed the unzip dependency by switching over to zipfile.

@srkukarni

This comment has been minimized.

Copy link
Contributor Author

srkukarni commented Sep 20, 2018

run java8 tests

@merlimat
Copy link
Contributor

merlimat left a comment

👍

@srkukarni

This comment has been minimized.

Copy link
Contributor Author

srkukarni commented Sep 20, 2018

run java8 tests

6 similar comments
@srkukarni

This comment has been minimized.

Copy link
Contributor Author

srkukarni commented Sep 21, 2018

run java8 tests

@srkukarni

This comment has been minimized.

Copy link
Contributor Author

srkukarni commented Sep 21, 2018

run java8 tests

@srkukarni

This comment has been minimized.

Copy link
Contributor Author

srkukarni commented Sep 21, 2018

run java8 tests

@srkukarni

This comment has been minimized.

Copy link
Contributor Author

srkukarni commented Sep 21, 2018

run java8 tests

@srkukarni

This comment has been minimized.

Copy link
Contributor Author

srkukarni commented Sep 21, 2018

run java8 tests

@srkukarni

This comment has been minimized.

Copy link
Contributor Author

srkukarni commented Sep 21, 2018

run java8 tests

@srkukarni

This comment has been minimized.

Copy link
Contributor Author

srkukarni commented Sep 21, 2018

run cpp tests

@srkukarni

This comment has been minimized.

Copy link
Contributor Author

srkukarni commented Sep 21, 2018

rum java8 tests

@srkukarni

This comment has been minimized.

Copy link
Contributor Author

srkukarni commented Sep 22, 2018

run java8 tests

2 similar comments
@srkukarni

This comment has been minimized.

Copy link
Contributor Author

srkukarni commented Sep 22, 2018

run java8 tests

@srkukarni

This comment has been minimized.

Copy link
Contributor Author

srkukarni commented Sep 23, 2018

run java8 tests

@srkukarni

This comment has been minimized.

Copy link
Contributor Author

srkukarni commented Sep 23, 2018

rerun tests

@sijie sijie modified the milestones: 2.2.0, 2.3.0 Sep 23, 2018

@srkukarni

This comment has been minimized.

Copy link
Contributor Author

srkukarni commented Sep 24, 2018

run cpp tests
run java8 tests

@srkukarni

This comment has been minimized.

Copy link
Contributor Author

srkukarni commented Sep 24, 2018

run cpp tests

@srkukarni srkukarni merged commit 200f97a into apache:master Sep 24, 2018

2 of 3 checks passed

Jenkins: Integration Tests FAILURE
Details
Jenkins: C++ / Python Tests SUCCESS
Details
Jenkins: Java 8 - Unit Tests SUCCESS
Details

@srkukarni srkukarni deleted the srkukarni:python_wheel branch Sep 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.