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

stream add-source file upload #39

Merged
merged 3 commits into from Feb 25, 2020
Merged

stream add-source file upload #39

merged 3 commits into from Feb 25, 2020

Conversation

Gerdie
Copy link
Contributor

@Gerdie Gerdie commented Feb 6, 2020

streaming to avoid issues related to running out of memory: #31

@@ -303,35 +302,41 @@ def validate_source(features):
@cli.command("add-source")
@click.argument("username", required=True, type=str)
@click.argument("id", required=True, type=str)
@cligj.features_in_arg
@click.argument("filename", required=True, type=str)
Copy link
Contributor

Choose a reason for hiding this comment

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

@Gerdie using @cligj means that we both:

  1. Can handle ld and non ld geojson (iow abstract this for the user)
  2. Validate and parse the geojson before passing in

I think we'll have to find a way to stream from input vs from a file. I think we have a few options:

  1. Stream in to temp file, stream temp file to upload
  2. cligj evaluates lazily so directly streaming in chunks (probably would be the weirdest to implement but most efficient overall)
  3. ?

Copy link
Contributor

@dnomadb dnomadb left a comment

Choose a reason for hiding this comment

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

I'd really like to keep cligj if possible, it handles a lot of abstraction for users and is a good "guard" for any programs that implement reading GeoJSON.

@Gerdie Gerdie requested a review from dnomadb February 6, 2020 22:06
Copy link
Contributor

@dnomadb dnomadb left a comment

Choose a reason for hiding this comment

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

This looks great, except that we'll need to add the requests_toolbelt dependency in setup.py

requirements.txt Show resolved Hide resolved
@Gerdie Gerdie requested a review from dnomadb February 17, 2020 23:00
@Gerdie Gerdie merged commit 71822f3 into master Feb 25, 2020
@Gerdie Gerdie deleted the upload_connection branch February 25, 2020 22:40
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.

None yet

2 participants