Fix CPDB nightly build #1405

sf-dcp · 2025-01-27T17:02:21Z

Closes #1399.

What

Nightly qa build for CPDB was failing, and this PR is meant to fix it. I recommend reviewing the GH issue linked above for more details.

Note: the build started passing again but it's the same behavior as in builds prior fails where doitt_buildingfootprints is empty prior the join. This PR is still very much needed.

Changes

Clean up input csv dcp_cpdb_agencyverified: remove ? from values in the bin column
Move pre-processing of doitt_buildingfootprints dataset from pandas to SQL and keep original raw version (previously it was dropped after pre-processing making it hard to debug)
- During the pre-processing step, the dataset is filtered where thebin column only has valid numeric values. Very basic.
- Why not to keep pre-processing in pandas? Because pandas changes the raw bin column to decimal (there are NULL values present) and then uploads the dataset back to database with the column as decimal. We do correct data types in ingest and this pandas behavior adds more complexity.
Update the join query between dcp_cpdb_agencyverified&doitt_buildingfootprintsto filter out invalidbin` values.

Successful build here.

fvankrieken · 2025-01-28T19:33:20Z

products/cpdb/bash/01_preprocessing.sh

-run_sql_command "DROP TABLE IF EXISTS doitt_buildingfootprints_source;"
+echo "fixing doitt_buildingfootprints_source and saving result into doitt_buildingfootprints"
+run_sql_command "
+    DROP TABLE IF EXISTS doitt_buildingfootprints;


I think I might lean dropping this entirely, and just handling null bins as needed. But I think that's getting into more of a general refactor - if we ever dbtify cpdb something like that can happen then

Yeah, we can drop this step. I don't think we need to handle the null bins

agree we can drop this. looks to me like attributes_agencyverified_geoms.sql handles null bins during the UPDATE dcp_cpdb_agencyverified part

and if this is dropped, than maybe there's no need to do import_as: doitt_buildingfootprints_source in the recipe?

@damonmcc yep, I will make the updates. Just waiting to meet with AD tmw to finalize this PR

fvankrieken · 2025-01-28T19:34:25Z

products/cpdb/data/dcp_cpdb_agencyverified.csv

@@ -1121,16 +1121,16 @@ Canceled - remove from map,DCAS,,,,, Fixed Asset, BPL - MILL BASIN BR LED RETROF
 ,DOHMH,,,,, Fixed Asset, APICHA COMMUNITY HEALTH CENTER: OUTFITTING OF CLINICAL EXAM  ,Pass-through,850HLQNAPICH,unmapped,,,
 ,DOHMH,,,,, , CATHOLIC MANAGED LONG TERM CARE (D/B/A ARCHCARE SENIOR LIFE  ,Pass-through,850HL82ARCHC,unmapped,,,
 ,DOHMH,,,,, Fixed Asset, DOH CLINIC CODE CORRECTIONS                                  ,Pass-through,81901199601,unmapped,,,
-385 Throop Ave.,DOHMH,3017960001.0,3050268???,Brooklyn,, Fixed Asset, DOHMH - LED LIGHTING UPGRADE (Bedford)                           ,Yes - Multi-site,816ACEDOH502,unmapped,,,11221.0


Have you investigated these bbls/bins at all? Do we know if this is a valid bin?

They do seem to all be 7 digits after removing question marks which at least means they're all theoretically valid bins now. I'm so fascinated by the question marks

@fvankrieken, great question. Yes, I did investigate them in doitt_buildingfootprints and found them to be valid bins but without a question mark in the end.

I think we might just want to check with AD if she has any idea why these were like this in the first place.

We have other invalid rows like

Which do have at least a valid BIN. Not that we should do anything about those right now, but maybe the question marks were included because they were unsure?? Just would be worth trying to make sure there's no reason/meaning. Don't see anything in the cpdb wiki

Would you like me to check with AD prior merging the PR?

fvankrieken · 2025-01-28T20:00:21Z

At some point we need to have an effort to go through our whole codebase and make sure bbls and bins are strings everywhere. But today is not that day.

sf-dcp · 2025-01-28T20:08:52Z

At some point we need to have an effort to go through our whole codebase and make sure bbls and bins are strings everywhere. But today is not that day.

Yeah, this should probably happen after we fully implement data checks for source data to validate data type (rn column data type in a ingest recipe is ignored)

- Keep raw dataset version - Do processing in database instead of pandas

The bin column in `dcp_cpdb_agencyverified` has invalid numeric values like "#REF!", "2610 / 336", and "3004301 (Additional BIN: 3829437 )". During the join, the table is filtered for decimal and integer values. Added casting to decimal because decimal string like "123.0" cannot be casted to integer directly

…ified` file"

sf-dcp · 2025-01-31T21:48:51Z

Met with AD: we decided to filter out all non-numerical bin values like 123??? or 123 (additional bin 321). I also created #1415.

CPDB run with new code updates here.

fvankrieken · 2025-02-03T15:06:31Z

So there are two points I can find that refer to doitt_buildingfootprints without either joining on bin column or specifying where bin is not null

Can you assess what impact that might have on these queries and any downstream logic?

fvankrieken · 2025-02-03T15:08:46Z

I'm running repeat builds of 24adopt on main and this branch and would like to look at the final tables and differences between them before merging. I can't imagine anything that would prevent merging, but just seems that we should assess impact before getting this fix on main

sf-dcp · 2025-02-03T17:00:18Z

So there are two points I can find that refer to doitt_buildingfootprints without either joining on bin column or specifying where bin is not null

https://github.com/NYCPlanning/data-engineering/blob/main/products/cpdb/sql/attributes_maprojid_bbl.sql#L43

https://github.com/NYCPlanning/data-engineering/blob/main/products/cpdb/sql/attributes_maprojid_bin.sql#L48

Can you assess what impact that might have on these queries and any downstream logic?

We haven't used doitt_buildingfootprints during CPDB at all since the table had been empty after its pre-processing step. Can you clarify how to assess the impact? @fvankrieken

fvankrieken · 2025-02-03T18:59:31Z

So there are two points I can find that refer to doitt_buildingfootprints without either joining on bin column or specifying where bin is not null

https://github.com/NYCPlanning/data-engineering/blob/main/products/cpdb/sql/attributes_maprojid_bbl.sql#L43

https://github.com/NYCPlanning/data-engineering/blob/main/products/cpdb/sql/attributes_maprojid_bin.sql#L48

Can you assess what impact that might have on these queries and any downstream logic?

We haven't used doitt_buildingfootprints during CPDB at all since the table had been empty after its pre-processing step. Can you clarify how to assess the impact? @fvankrieken

Sorry - we made a decision to not filter where bin is not null, I am referring to the impact of that decision

sf-dcp · 2025-02-03T21:09:49Z

@fvankrieken, the most recent doit_buildingfootprints version has a total of 2 records with null bins (geometry is not null). So the impact is not dramatic. But it makes sense to add a clause 'where bin is not null' just in case, before we add actual data checks.

…om is null

sf-dcp · 2025-02-05T16:31:04Z

@fvankrieken, filtering for non-null geoms in doitt_buildingfootprints didn't solve the problem: the projects 846P-4OLMST3 & 846P-400ZOO8 still got a NULL value. No datasource is present.

When you were comparing the tables between my older branch and main, was the datasource doitt_buildingfootprints present?

fvankrieken · 2025-02-05T17:08:38Z

@fvankrieken, filtering for non-null geoms in doitt_buildingfootprints didn't solve the problem: the projects 846P-4OLMST3 & 846P-400ZOO8 still got a NULL value. No datasource is present.

When you were comparing the tables between my older branch and main, was the datasource doitt_buildingfootprints present?

Did you repeat 24adopt, or build with latest data? I repeated 24adopt, so if you built with latest there's a chance things have changed. Given that you now have no datasource I would guess this might be the case and that your code changes are working as expected. However, you should repeat 24adopt to make sure

This is what I saw on my runs - all rows that are getting assigned "footprint_script" as source on dev branch shown below

maprojid	description	dev_geom_source	main_geom_source	dev_geom	main_geom
846P-4OLMST3	OLMSTED CTR FURNITURE Q099-808MA1	footprint_script	dpr		ST_MultiPolygon
846SANDY3-14	SANDY-Dock at 79 Street Boat Basin M071-310M	footprint_script	dpr	ST_MultiPoint	ST_MultiPolygon
846P-400ZOO8	Queens Zoo Education Classroom Construction Q099-123M	footprint_script	dpr		ST_MultiPolygon
846P-4OLMST	OLMSTED CTR RC AND ANNEX DESIGN FEE Q099-808M/1108M	footprint_script	dpr
846P-415PAVI	FMCP - NYS PAVILION RC Q099-116M	footprint_script	dpr	ST_MultiPoint	ST_MultiPolygon
846SANDY2-14	SANDY- Abe Stark boilers and chillers (B169-111M)	footprint_script	dpr	ST_MultiPoint	ST_MultiPolygon
846SANDY4-21	SANDY-FOREST PARK GREENHOUSE BROKEN GLASS REPLACEMENT	footprint_script	Algorithm
846P-415LWPG	FMCP - Lawrence Plgd & PRB R/C Q099-216M- DESIGN	footprint_script	dpr
846P-200CRSL	PPA- Recon. Carousel (B073-218M)	footprint_script	dpr	ST_MultiPoint	ST_MultiPolygon
846P-200PPOP	PPA - Prospect Park Oriental Pavilion B073-416M	footprint_script	dpr	ST_MultiPoint	ST_MultiPolygon
846P-200WCST	PPA-Wellhouse Public Rest Room Renovation - (B073-209M)	footprint_script	dpr	ST_MultiPoint	ST_MultiPolygon
846P-200PPPH	PPA- PROSPECT PARK PICNIC HOUSE RENOVATION B073-419M	footprint_script	dpr	ST_MultiPoint	ST_MultiPolygon
846P-200LFHH	PPA- Restor. Lefferts Historic House B073-317M	footprint_script	dpr	ST_MultiPoint	ST_MultiPolygon
846P-313CPSW	Central Park - Swedish Cottage M010-116M	footprint_script	dpr
846P-200BAND	PPA- RC Prospect Park Bandshell - equipment Purch.	footprint_script	Algorithm	ST_MultiPoint	ST_MultiPolygon
126PV274PERI	NYHOS - New York Hall of Science, Perimeter Landscaping	footprint_script	Facilities database	ST_MultiPoint	ST_MultiPoint

sf-dcp force-pushed the sf-debug-cpdb branch 7 times, most recently from 5835d7c to fb996d8 Compare January 28, 2025 19:24

sf-dcp marked this pull request as ready for review January 28, 2025 19:30

sf-dcp assigned damonmcc, fvankrieken and alexrichey Jan 28, 2025

fvankrieken reviewed Jan 28, 2025

View reviewed changes

sf-dcp added 4 commits January 31, 2025 16:35

Fix processing of doitt_buildingfootprints:

680fe1b

- Keep raw dataset version - Do processing in database instead of pandas

Fix invalid bins in ddcp_cpdb_agencyverified file

b6ce7bc

POST REVIEW: remove pre-processing of doitt_buildingfootprints

5078b80

sf-dcp force-pushed the sf-debug-cpdb branch from 025cea6 to 5078b80 Compare January 31, 2025 21:36

POST REVIEW with AD: Revert "Fix invalid bins in d`dcp_cpdb_agencyver…

3ae505c

…ified` file"

sf-dcp requested review from fvankrieken and damonmcc January 31, 2025 22:03

POST REVIEW: clean bin null records for doitt_buildingfootprints

aee3159

sf-dcp added 2 commits February 3, 2025 16:28

small oopsi

7367a0b

POST REVIEW: do not update geom column if doitt_buildingfootprints.ge…

999f041

…om is null

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix CPDB nightly build #1405

Fix CPDB nightly build #1405

sf-dcp commented Jan 27, 2025 •

edited

Loading

fvankrieken Jan 28, 2025

sf-dcp Jan 28, 2025 •

edited

Loading

damonmcc Jan 30, 2025

sf-dcp Jan 30, 2025

fvankrieken Jan 28, 2025

fvankrieken Jan 28, 2025

sf-dcp Jan 28, 2025

fvankrieken Jan 28, 2025

sf-dcp Jan 28, 2025

fvankrieken commented Jan 28, 2025

sf-dcp commented Jan 28, 2025

sf-dcp commented Jan 31, 2025 •

edited

Loading

fvankrieken commented Feb 3, 2025

fvankrieken commented Feb 3, 2025

sf-dcp commented Feb 3, 2025 •

edited

Loading

fvankrieken commented Feb 3, 2025

sf-dcp commented Feb 3, 2025

sf-dcp commented Feb 5, 2025

fvankrieken commented Feb 5, 2025

Fix CPDB nightly build #1405

Are you sure you want to change the base?

Fix CPDB nightly build #1405

Conversation

sf-dcp commented Jan 27, 2025 • edited Loading

What

Changes

Choose a reason for hiding this comment

sf-dcp Jan 28, 2025 • edited Loading

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

fvankrieken commented Jan 28, 2025

sf-dcp commented Jan 28, 2025

sf-dcp commented Jan 31, 2025 • edited Loading

fvankrieken commented Feb 3, 2025

fvankrieken commented Feb 3, 2025

sf-dcp commented Feb 3, 2025 • edited Loading

fvankrieken commented Feb 3, 2025

sf-dcp commented Feb 3, 2025

sf-dcp commented Feb 5, 2025

fvankrieken commented Feb 5, 2025

sf-dcp commented Jan 27, 2025 •

edited

Loading

sf-dcp Jan 28, 2025 •

edited

Loading

sf-dcp commented Jan 31, 2025 •

edited

Loading

sf-dcp commented Feb 3, 2025 •

edited

Loading