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

Feature/aws improvements #336

Merged
merged 16 commits into from
Nov 22, 2019
Merged

Feature/aws improvements #336

merged 16 commits into from
Nov 22, 2019

Conversation

mlshapiro
Copy link
Contributor

@mlshapiro mlshapiro commented Nov 12, 2019

  • Lambda node by default uses "invoke" instead of S3. Includes handler improvements to consolidate the "pipeline" data model that is passed among the various triggers.
  • Data is passed back and forth via netcdf.
  • Errors are returned via a LambdaException with stack trace (this is awesome).
  • Lambda function can be restricted to only allow certain Nodes using function_restrict_pipelines. This uses an environment variable (PODPAC_RESTRICT_PIPELINES) in the lambda function to store the hashes of acceptable Nodes. The handler checks to see if the node hash is in the environment variable and raises a ValueError if not.

API Gateway and S3 triggers still needs testing. Handler may need to be updated for these as well.

In the future, I think we should deprecate the "eval_s3" and "eval_api" methods on the Lambda node since these are no longer relevant. We should keep the ability to add extra triggers for S3 and APIGateway since there are other use cases that need these.

To Do:

  • Parse the response["LogResult"] to get the "REPORT" which contains billing information
    • Perhaps we want to skip this, seems like it would be complicated to keep track of every node's billing information. Probably want to add a boto3 integration separately for this
  • Test the APIGateway examples using the requests library.
    • Make sure the WMS urls are getting parsed correctly.
    • Confirm that errors are getting passed back correctly
  • Test the S3 trigger outside of podpac.
    • Make sure that the output file is put into the specified output directory

Lambda node by default uses "invoke" instead of S3. Includes handler improvements to consolidate the "pipeline" data model that is passed amongst the various triggers.
Data is passed back and forth via netcdf.

API Gateway and S3 triggers still needs testing.

In the future, I think we should deprecate the "eval_s3" and "eval_api" methods since these are no longer relevant. We should keep the ability to add extra triggers for S3 and APIGateway since there may be other use cases that need this, but the PODPAC only evaluation will be done through invoke.
@mlshapiro mlshapiro requested a review from dsully-dev November 12, 2019 22:26
@mlshapiro mlshapiro self-assigned this Nov 12, 2019
@coveralls
Copy link

coveralls commented Nov 13, 2019

Coverage Status

Coverage decreased (-0.5%) to 81.678% when pulling 41a25cd on feature/aws-improvements into c5fb3fb on develop.

@@ -173,6 +181,13 @@ def _function_name_default(self):

return settings["FUNCTION_NAME"]

@tl.default("function_triggers")
Copy link
Contributor

@dsully-dev dsully-dev Nov 15, 2019

Choose a reason for hiding this comment

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

@mlshapiro This block seems to indicate I must set function_eval_trigger to 'APIGateway' in order to build the api gateway. More generally, it is not obvious to me how to specify that I want optional resources built (e.g. API Gateway).


pipeline = default_pipeline()
pipeline["url"] = event["queryStringParameters"]
pipeline["params"] = urllib.parse_qs(urllib.urlparse(pipeline["url"]).query)
Copy link
Contributor

Choose a reason for hiding this comment

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

We actually don't need to do any URL parsing, because event['queryStringParameters'] is a dict with all of the params already parsed.


url = event["queryStringParameters"]
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like it was in the porting of this block that we lost the isinstance check.

David Sullivan and others added 4 commits November 18, 2019 16:26
The `queryStringParameters` are already a dict, so they don't need parsed with `urllib`.

This also merges any settings passed in the event with the default pipeline, then again with the settings on the AWS instance.
@dsully-dev dsully-dev merged commit 3c52a2b into develop Nov 22, 2019
@dsully-dev dsully-dev deleted the feature/aws-improvements branch November 22, 2019 14:58
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.

4 participants