-
Notifications
You must be signed in to change notification settings - Fork 0
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
GFT tweaks for 24v3 draft 1 review #1196
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1196 +/- ##
=======================================
Coverage 68.60% 68.60%
=======================================
Files 109 109
Lines 5771 5771
Branches 634 634
=======================================
Hits 3959 3959
Misses 1685 1685
Partials 127 127 ☔ View full report in Codecov by Sentry. |
4b5faa5
to
721e7ae
Compare
721e7ae
to
1ab6bc9
Compare
@@ -341,30 +341,32 @@ def _cli_wrapper_repeat_recipe( | |||
), | |||
): | |||
product_key: publishing.BuildKey | publishing.DraftKey | publishing.PublishKey | |||
|
|||
product_label = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on just passing along a --product-label
CLI option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like that would make sense, though then we'd need to have this sort of logic in the repeat
gha - still need to refer to both formattings in our code at the moment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I guess I'm angling for a way to avoid hardcoding these product overrides, which feels like it's going in the wrong direction. I'm fine if we just want to punt though until we have a strong feeling about where's best for this type of thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. Yeah I feel like a CLI option isn't really great either, in that it still adds some unneeded complexity. But it's 100% a lesser evil than what's in this PR
Maybe we should just have some sort of minimal ProductPaths
object? And then dcpy can rely on pyproject.toml (or .dcpy
file, etc) that defines the path to product folder, DO folder name, db build name, etc. OR for each product, we have a standard spot to look, (i.e. the product folder) and store the additional metadata there. This seems like this could belong in a recipe actually. Though then you would need the recipe to run an upload step.
Long way of saying I think we should punt the issue. I can make an issue for this
@fvankrieken Do we need to do this for all |
We need to do this for every dataset that starts out as points and attempts to join to lots (so final geom column contains both points and polys). Industrial lots has pluto as a source, so every row is a lot from the start |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open question about hard-coding overrides. Seems less than ideal, but I'm fine if we to address that later.
Tweaks for #1004 (comment)
rebuild here
Next draft is in QA, but I think it makes sense to get this merged in the meantime and follow-up as needed.
Changes are
points
andlots
source dataset outputs that thelots
only contain rows where the geom is not null