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

Remove support for pfr<=0.9.x #170

Merged
merged 5 commits into from
Feb 15, 2024
Merged

Conversation

moradology
Copy link
Contributor

This draft aims to move us towards cutting support for the older to_beam recipe version. This ought to simplify developer work and cut down on CI complexity. #144

Copy link

codecov bot commented Feb 12, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (8e8e4be) 96.46% compared to head (626a5d9) 95.93%.
Report is 7 commits behind head on main.

Files Patch % Lines
pangeo_forge_runner/commands/bake.py 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #170      +/-   ##
==========================================
- Coverage   96.46%   95.93%   -0.53%     
==========================================
  Files          15       14       -1     
  Lines         509      492      -17     
==========================================
- Hits          491      472      -19     
- Misses         18       20       +2     

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

@moradology moradology changed the title Remove support for pfr<=0.10.x Remove support for pfr<=0.9.x Feb 12, 2024
Copy link
Collaborator

@ranchodeluxe ranchodeluxe 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 good to me and once in unblocks some good work in:
#171
#166

@moradology
Copy link
Contributor Author

@cisaacstern It occurs to me you might have an opinion here. I'm for pulling the trigger and moving things forward as i think there are further issues with the ways recipes are handled that will require potential further deprecation or at least heavy guidance on the semantics of the two approaches - PTransform construction vs lazily building our full pipeline inside a function that will serve as the recipe (this is enabled by the PR about lazy pipeline construction #168)

@cisaacstern
Copy link
Member

Thanks for the ping I will take a look this afternoon.

@cisaacstern cisaacstern added test-dataflow Add this label to PRs to trigger Dataflow integration test. test-flink Add this label to PRs to trigger Dataflow integration test. labels Feb 14, 2024
Copy link
Member

@cisaacstern cisaacstern left a comment

Choose a reason for hiding this comment

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

Overall LGTM, @moradology, thanks so much for doing this overdue maintenance! One other thing we should remove as part of this PR is:

class MetadataCacheStorage(StorageTargetConfig):
"""
Storage configuration for caching metadata during recipe baking
"""
pangeo_forge_target_class = "MetadataTarget"

Since that is only relevant in pre-0.10.0 releases.

Apart from that, I haven't taken a close look yet as to why codecov is upset, but if possible would be great to get that passing.

@ranchodeluxe
Copy link
Collaborator

ranchodeluxe commented Feb 14, 2024

well, codecov was complaining b/c the number of removals seemed to reduce the overall ratio of coverage, not b/c important things changed. I added some codecov.yml rules that exclude /tests/ and obvious things that the recipes repo excluded and we're gravy 🥳

gonna merge this sucker

@cisaacstern
Copy link
Member

Awesome! Thanks all!

@ranchodeluxe ranchodeluxe merged commit 028903c into main Feb 15, 2024
27 of 28 checks passed
@moradology moradology deleted the npz/feature/cut-0.9recipe-support branch February 16, 2024 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test-dataflow Add this label to PRs to trigger Dataflow integration test. test-flink Add this label to PRs to trigger Dataflow integration test.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants