ingest - implement and make use of connector classes #1519

fvankrieken · 2025-03-12T16:58:29Z

Multiple things big and small going on here. I tried to be quite detailed in commit descriptions, so look there as you go by commit. Definitely go by commit, I think it's relatively sane

all the connector-related ingest changes are in one commit (9th) - if you're unsure of how the functionality in a specific commit is relevant, I recommend checking that one as a reference.

Main takeaways

rework connectors a bit, generally
- different apis that can be available: versioned vs nonversioned, etc
- move towards kwargs instead of dict object passed in
- registry "subregistries" so that you can declare what api is needed at various points in codebase
implement connectors for storage in ingest. For now, this is separate from edm.recipes, which is a little more coupled to being backwards compatible with library
use connectors in ingest

My biggest open questions

Resolve edm.recipes vs ingest_datastore -> should this new connector just go in recipes? Should it become one with the current recipes connector?
- arg against -> nice to have clean break. An external user stays further away from any code that has a concept of library. Also don't have to worry about refactoring name (ingest_datastore is generic, better to expose slightly to users, whereas edm.recipes is very much OUR datastore)
- arg for -> might be easier to take advantages of the new connector if we just make one and have it backwards compatible. Less room for confusion moving forward with two parallel connectors, one pushing one pulling.
handling raw filename in ingest (see comment)

codecov · 2025-04-02T21:27:18Z

Codecov Report

Attention: Patch coverage is 91.47465% with 37 lines in your changes missing coverage. Please review.

Project coverage is 71.19%. Comparing base (a71b5bf) to head (0224d21).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
dcpy/connectors/registry.py	83.52%	12 Missing and 2 partials ⚠️
dcpy/connectors/edm/publishing.py	79.54%	8 Missing and 1 partial ⚠️
dcpy/connectors/filesystem.py	91.17%	1 Missing and 2 partials ⚠️
dcpy/connectors/ingest_datastore.py	93.75%	2 Missing and 1 partial ⚠️
dcpy/lifecycle/data_loader.py	0.00%	3 Missing ⚠️
dcpy/models/lifecycle/ingest.py	94.73%	3 Missing ⚠️
dcpy/connectors/edm/recipes.py	88.88%	1 Missing ⚠️
dcpy/connectors/esri/arcgis_feature_service.py	95.45%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1519      +/-   ##
==========================================
+ Coverage   70.69%   71.19%   +0.50%     
==========================================
  Files         125      129       +4     
  Lines        6504     6690     +186     
  Branches      742      732      -10     
==========================================
+ Hits         4598     4763     +165     
- Misses       1750     1775      +25     
+ Partials      156      152       -4

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

If we have different apis for these connectors, at some points we need to be able to declare what we expect to be able to do. Maybe this is a little stringent right now (before getting a subregistry of our default one, no api is available), so might need some reworking. I think the utility of this is more shown when being used in ingest.

Implement connector objects and tests for the various ingest sources

two points here - "ingest_datastore" connector that's decoupled from any specific storage. The idea is that user could set up storage for ingest with something like what's sort of outlined in #1555 - "storage" connectors underneath - I really needed a connector that both was non-versioned and also had some concept of listing things after a prefix. This maybe will be scrapped in the future. Alex and I have talked about how maybe all connectors should move towards "paths" (sort of RESTful) which would make this maybe redundant. But for now, I needed it.

reduce boilerplate. this could have gone either direction, but there were cases where "pull_versioned" needed a specific implementation so this felt more generalized.

sf-dcp · 2025-04-17T13:49:44Z

dcpy/lifecycle/ingest/validate.py

-def validate_against_existing_versions(
-    ds: recipes.Dataset, filepath: Path
-) -> ArchiveAction:
+def validate_against_existing_versions(ds: recipes.Dataset, filepath: Path) -> bool:


can we rename this function to something like is_new_version and also update the doctsring where it says that the config file will be updated in line 16? From my understanding, the config file will no longer record the date checked.

Oh wow the docstring is way inaccurate. Yeah will update.

Maybe compare_to_existing_version? is_equivalent_to_existing_version? I think the former maybe

I guess the scope is more "check if it's new (against the existing versions in recipes), then if it isn't validate the data"... which sort of leads me back to the current name. But to that point, I think the scope of this function is a little confusing.

It also has a dependency on recipes which needs to be removed. Let me add a commit to try to clean this up and we can see if we think it belongs or if it's a little too weird to try to fit into this PR

sf-dcp

Reviewed at high level -- LGTM!

github-project-automation bot added this to Data Engineering Mar 12, 2025

github-project-automation bot moved this to New in Data Engineering Mar 12, 2025

fvankrieken force-pushed the fvk-ingest-connectors branch 8 times, most recently from 5660ddf to 0d10fd6 Compare March 14, 2025 23:33

fvankrieken changed the base branch from main to fvk-ingest-s3opt March 14, 2025 23:33

Base automatically changed from fvk-ingest-s3opt to main March 18, 2025 01:58

fvankrieken force-pushed the fvk-ingest-connectors branch 12 times, most recently from 535a463 to 39ec8d0 Compare March 21, 2025 14:01

fvankrieken force-pushed the fvk-ingest-connectors branch 5 times, most recently from 2d3b04a to daa5c27 Compare April 2, 2025 21:22

fvankrieken added 5 commits April 15, 2025 17:21

add source connectors for ingest

58c3962

Implement connector objects and tests for the various ingest sources

chk - ingest

3638ee7

default push/pull implementation for versioned

9e017c7

reduce boilerplate. this could have gone either direction, but there were cases where "pull_versioned" needed a specific implementation so this felt more generalized.

fvankrieken force-pushed the fvk-ingest-connectors branch 3 times, most recently from d7e8026 to 5c69e2a Compare April 15, 2025 21:26

fvankrieken added 3 commits April 15, 2025 17:40

POST-REVIEW: deprecate 'Type'

be969ee

POST-REVIEW: rename 'drive'

90230b3

POST-REVIEW: just pass storage to ingest conn

7c1ad6c

fvankrieken force-pushed the fvk-ingest-connectors branch from 5c69e2a to 7c1ad6c Compare April 15, 2025 21:41

collision - update literals

74c5a3d

sf-dcp self-requested a review April 16, 2025 14:59

sf-dcp reviewed Apr 17, 2025

View reviewed changes

fvankrieken force-pushed the fvk-ingest-connectors branch 4 times, most recently from 9656b3e to bf482ac Compare April 17, 2025 16:12

sf-dcp approved these changes Apr 17, 2025

View reviewed changes

fvankrieken force-pushed the fvk-ingest-connectors branch 2 times, most recently from 91bd355 to 25d385f Compare April 17, 2025 17:22

POST-REVIEW: logic around existing ingest versions

25ada63

fvankrieken force-pushed the fvk-ingest-connectors branch from 25d385f to 25ada63 Compare April 17, 2025 17:25

fvankrieken added 2 commits April 17, 2025 13:26

drop 's'

ece1940

slightly different mypy hack for abstract generics

0224d21

fvankrieken merged commit 6add0c5 into main Apr 17, 2025
23 checks passed

github-project-automation bot moved this from New to Done in Data Engineering Apr 17, 2025

fvankrieken deleted the fvk-ingest-connectors branch April 17, 2025 18:52

fvankrieken mentioned this pull request Apr 21, 2025

Fix extension for socrata pull #1614

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

ingest - implement and make use of connector classes #1519

ingest - implement and make use of connector classes #1519

Uh oh!

fvankrieken commented Mar 12, 2025 •

edited

Loading

Uh oh!

codecov bot commented Apr 2, 2025 •

edited

Loading

Uh oh!

sf-dcp Apr 17, 2025

Uh oh!

fvankrieken Apr 17, 2025

Uh oh!

fvankrieken Apr 17, 2025

Uh oh!

sf-dcp left a comment

Uh oh!

Uh oh!

Uh oh!

ingest - implement and make use of connector classes #1519

ingest - implement and make use of connector classes #1519

Uh oh!

Conversation

fvankrieken commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

all the connector-related ingest changes are in one commit (9th) - if you're unsure of how the functionality in a specific commit is relevant, I recommend checking that one as a reference.

Uh oh!

codecov bot commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sf-dcp Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

fvankrieken Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

fvankrieken Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

sf-dcp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fvankrieken commented Mar 12, 2025 •

edited

Loading

codecov bot commented Apr 2, 2025 •

edited

Loading