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

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 and others added 2 commits September 17, 2018 09:40
*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))
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

@jerrypeng Have removed the string convertor.
Also I have removed dependence on virtualenv and made the patch a lot simpler

Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

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",
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@srkukarni
Copy link
Contributor Author

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

Copy link
Contributor

@ivankelly ivankelly left a comment

Choose a reason for hiding this comment

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

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)))
Copy link
Contributor

Choose a reason for hiding this comment

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

unzip is a new dependency.

Copy link
Contributor

Choose a reason for hiding this comment

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

where is this unzipped to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unzip is indeed a new dependency.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@srkukarni
Copy link
Contributor Author

run java8 tests

Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

👍

@srkukarni
Copy link
Contributor Author

run java8 tests

6 similar comments
@srkukarni
Copy link
Contributor Author

run java8 tests

@srkukarni
Copy link
Contributor Author

run java8 tests

@srkukarni
Copy link
Contributor Author

run java8 tests

@srkukarni
Copy link
Contributor Author

run java8 tests

@srkukarni
Copy link
Contributor Author

run java8 tests

@srkukarni
Copy link
Contributor Author

run java8 tests

@srkukarni
Copy link
Contributor Author

run cpp tests

@srkukarni
Copy link
Contributor Author

rum java8 tests

@srkukarni
Copy link
Contributor Author

run java8 tests

2 similar comments
@srkukarni
Copy link
Contributor Author

run java8 tests

@srkukarni
Copy link
Contributor Author

run java8 tests

@srkukarni
Copy link
Contributor Author

rerun tests

@sijie sijie modified the milestones: 2.2.0, 2.3.0 Sep 23, 2018
@srkukarni
Copy link
Contributor Author

run cpp tests
run java8 tests

@srkukarni
Copy link
Contributor Author

run cpp tests

@srkukarni srkukarni merged commit 200f97a into apache:master Sep 24, 2018
@srkukarni srkukarni deleted the python_wheel branch September 24, 2018 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants