GTC-2618 Allow appending layers to existing tables #525

manukala6 · 2024-05-21T19:18:27Z

Pull request checklist

Please check if your PR fulfills the following requirements:

Make sure you are requesting to pull a topic/feature/bugfix branch (right side). Don't request your master!
Make sure you are making a pull request against the develop branch (left side). Also you should start your branch off our develop.
Check the commit's or even all commits' message styles matches our requested structure.
Check your code additions will fail neither code linting checks nor unit test.

Pull request type

Please check the type of change your PR introduces:

What is the current behavior?

Issue Number: GTC-2618

What is the new behavior?

Append layers or static files to existing tables on the API
Remove deprecated status for append to version route
Handle appends to test files (gfw-datapump updates) differently from other appends

Does this introduce a breaking change?

Yes
No

Other information

Develop

…ersion) We still don't have permissions correct for doing the needed PutBuckedLifecycleConfiguration operation done by expire. And the failed background job is causing the main delete request to fail with a 504. (Same for deleting tile caches directly.)

Disable expire_s3_objects call in delete_all_assets (used by delete_version)

Merge to production

Make name field optional in User model

codecov-commenter · 2024-06-13T06:08:25Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 70.96774% with 9 lines in your changes missing coverage. Please review.

Project coverage is 81.96%. Comparing base (e5140f8) to head (f75a142).

Files	Patch %	Lines
app/routes/assets/asset.py	41.66%	7 Missing ⚠️
app/routes/datasets/versions.py	83.33%	2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #525      +/-   ##
===========================================
- Coverage    82.03%   81.96%   -0.07%     
===========================================
  Files          126      126              
  Lines         5665     5689      +24     
===========================================
+ Hits          4647     4663      +16     
- Misses        1018     1026       +8

Flag	Coverage Δ
unittests	`81.96% <70.96%> (-0.07%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

danscales

Are you missing some other test changes? It seems like you added test.gpkg.zip, but then didn't use it. Also, I was expecting another append test that explicitly tests your new code.

danscales · 2024-06-14T17:56:27Z

app/routes/datasets/versions.py

+        elif request.layers is not None:
+            input_data["creation_options"]["layers"] += request.layers
+        else:
+            input_data["creation_options"]["layers"] = request.layers


I don't understand this one. The original dataset does have a layers value, so why are you then replacing input_data["creation_options"]["layers"] with the request.layers value which is equal to None? It seems like you want to either have an error if request.layers is None or leave the current layers the same, right? Or can you add a comment here on why you are setting layers to None?

input_data["creation_options"] is the body for the append POST request, so we want to exclude the layers from the version creation creations for that operation. Ln 268-76 are updating the creation options object for the version itself. I added comments to reflect this.

app/routes/datasets/versions.py

danscales · 2024-06-14T18:18:03Z

app/routes/datasets/versions.py

+    update_data["creation_options"]["source_uri"] += request.source_uri # ERROR: only one source_uri is allowed
+    if input_data["creation_options"].get("layers") is not None:
+        if update_data["creation_options"]["layers"] is not None:
+            update_data["creation_options"]["layers"] += request.layers


If request.layers is None (which you check for explicitly at line 256 above), then you will get a Python error here when you try add None (which is not a list) to an existing list.

I refactored this section to account for this. None is now not being added to a list.

danscales · 2024-06-14T18:21:31Z

terraform/vars/terraform-production.tfvars

@@ -5,6 +5,8 @@ rw_api_url                = "https://api.resourcewatch.org"
 desired_count             = 2
 auto_scaling_min_capacity = 2
 auto_scaling_max_capacity = 15
+fargate_cpu               = 2048
+fargate_memory            = 4096


I guess this is OK, but somehow you are including hotfixes from master into your change?

dmannarino · 2024-06-14T20:29:46Z

app/models/pydantic/creation_options.py

-                len(v) == 1
-            ), "Non-CSV vector sources require one and only one input file"
+        elif values.get("source_driver") in [VectorDrivers.esrijson, VectorDrivers.shp, VectorDrivers.geojson_seq, VectorDrivers.geojson]:
+            assert (len(v) == 1), "GeoJSON and ESRI Shapefile vector sources require one and only one input file"


I think you're intending for this change to allow multiple input files for GPKG, but is that actually supported in the code? Maybe I'm looking in the wrong place, but in vector_source_assets.py it looks like we currently only make use of the first source_uri (perhaps because of the issues that might arise from specifying layers for multiple files?): https://github.com/wri/gfw-data-api/blob/master/app/tasks/vector_source_assets.py#L254

This wasn't for allowing multiple input files, rather its for updating the version creation options after the append operation is successful (there would be more than 1 source_uri in this case)

Okay, I see. But would it have the side effect of now allowing multiple GPKG sources to be specified, though those after the first will be silently ignored?

dmannarino · 2024-06-14T20:34:49Z

app/models/pydantic/versions.py

+        None,
+        description="List of layer names to append to version. "
+        "If not set, all layers in source_uri will be appended.",
+    )


It looks to me like if layers is not specified it will be assumed that there is only a layer named like the file (though I may be looking in the wrong place in the code): https://github.com/wri/gfw-data-api/blob/master/app/tasks/vector_source_assets.py#L209-L214

Good catch, I corrected the description. Layer names are required for .GDB and .GPKG, otherwise the file name is used as the layer name.

app/routes/datasets/versions.py

solomon-negusse · 2024-06-17T10:39:01Z

app/routes/datasets/versions.py

+    # If source_driver is "text", this is a datapump request
+    if input_data["creation_options"]["source_driver"] != "text":
+        # Verify that source_driver is not None
+        if input_data["creation_options"]["source_driver"] is None:


The pydanic model for appending should be updated to take in creation_options (or update_options), right? That'd be the right place to validate input like so:

solomon-negusse

@manukala6, i left some comments, mainly I'd encourage updating the append pydantic model to include creation options to handle validation.

solomon-negusse · 2024-06-17T10:56:53Z

app/routes/datasets/versions.py

+            )
+
+        # Append the new layers to the existing ones
+        if input_data["creation_options"].get("layers") is None: # ERROR: layers is not defined


Maybe meant this to be:
if input_data["creation_options"].get("layers") is None and request.layers is not None?

Another way to simplify the whole logic could be:

if request.layers: if input_data["creation_options"].get("layers") is None: input_data["creation_options"]["layers"] += request.layers else: input_data["creation_options"]["layers"] += request.layers

Another alternative is to set the default layers value to [] but would be good to check that doesn't introduce breaking changes.

Thanks for the input, I refactored it similarly.

app/routes/datasets/versions.py

solomon-negusse and others added 16 commits May 1, 2024 10:39

Merge pull request #499 from wri/develop

ebfe539

Develop

Merge pull request #511 from wri/gtc-2708d

2d2a5be

Disable expire_s3_objects call in delete_all_assets (used by delete_version)

Merge pull request #520 from wri/develop

6ce63f9

Merge to production

Make name field optionl in User model

3d1cd5b

Merge pull request #521 from wri/gtc-2833/optional_name

fcbf56a

Make name field optional in User model

GTC-2618 Allow appends of GPKG layers to table

218b11f

increase production fargate task size

c9ee67d

Add tiles_info endpoint

65c496c

GTC-2618 Fix tests

0219171

Merge branch 'develop' into feature/vector_layer_appends

5d3817d

GTC-2618 Fix tests

cda9016

GTC-2618 Fix test creation options

ebf854c

GTC-2618 Fix test creation options

ddb66d8

GTC-2618 Add exception for datapump appends

13d1145

GTC-2618 Remove missing layers test

a5509f1

manukala6 marked this pull request as ready for review June 13, 2024 15:59

Merge branch 'develop' into feature/vector_layer_appends

4edd3b4

manukala6 changed the base branch from master to develop June 13, 2024 16:10

manukala6 changed the title ~~GTC-2618 Allow appends of GPKG layers to table~~ GTC-2618 Allow appending layers to existing tables Jun 13, 2024

manukala6 requested review from jterry64, danscales, solomon-negusse and dmannarino June 13, 2024 16:38

danscales reviewed Jun 14, 2024

View reviewed changes

dmannarino reviewed Jun 14, 2024

View reviewed changes

solomon-negusse reviewed Jun 17, 2024

View reviewed changes

app/routes/datasets/versions.py Outdated Show resolved Hide resolved

solomon-negusse reviewed Jun 17, 2024

View reviewed changes

solomon-negusse requested changes Jun 17, 2024

View reviewed changes

manukala6 added 6 commits June 21, 2024 15:39

GTC-2618 Clean up version append

4e52f20

GTC-2618 Remove source_driver requirement

957023e

GTC-2618 Add tests for append

0608757

GTC-2618 Simplify append logic

b414ea1

GTC-2618 Update tests_v2

5013b66

Merge branch 'develop' into feature/vector_layer_appends

5c478d1

manukala6 requested review from danscales, dmannarino and solomon-negusse June 27, 2024 17:22

danscales approved these changes Jun 28, 2024

View reviewed changes

app/routes/datasets/versions.py Show resolved Hide resolved

solomon-negusse approved these changes Jul 1, 2024

View reviewed changes

manukala6 and others added 2 commits July 1, 2024 09:19

Merge branch 'develop' into feature/vector_layer_appends

63ae482

Merge branch 'develop' into feature/vector_layer_appends

f75a142

manukala6 merged commit b568edc into develop Jul 1, 2024
2 checks passed

manukala6 deleted the feature/vector_layer_appends branch July 1, 2024 17:12

manukala6 mentioned this pull request Jul 1, 2024

Merge to production #550

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GTC-2618 Allow appending layers to existing tables #525

GTC-2618 Allow appending layers to existing tables #525

manukala6 commented May 21, 2024 •

edited

Loading

codecov-commenter commented Jun 13, 2024 •

edited

Loading

danscales left a comment

danscales Jun 14, 2024

manukala6 Jun 27, 2024

danscales Jun 14, 2024

manukala6 Jun 27, 2024

danscales Jun 14, 2024

dmannarino Jun 14, 2024

manukala6 Jun 27, 2024

dmannarino Jul 1, 2024

dmannarino Jun 14, 2024

manukala6 Jun 27, 2024

solomon-negusse Jun 17, 2024 •

edited

Loading

solomon-negusse left a comment

solomon-negusse Jun 17, 2024

manukala6 Jun 27, 2024

GTC-2618 Allow appending layers to existing tables #525

GTC-2618 Allow appending layers to existing tables #525

Conversation

manukala6 commented May 21, 2024 • edited Loading

Pull request checklist

Pull request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Other information

codecov-commenter commented Jun 13, 2024 • edited Loading

Codecov Report

danscales left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solomon-negusse Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

solomon-negusse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manukala6 commented May 21, 2024 •

edited

Loading

codecov-commenter commented Jun 13, 2024 •

edited

Loading

solomon-negusse Jun 17, 2024 •

edited

Loading