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

Add jobs for excluding EPA CEMS assets #2343

Merged
merged 3 commits into from
Mar 3, 2023

Conversation

bendnorman
Copy link
Member

PR Overview

I realized we often run the ETL without EPA CEMS so it doesn't take forever. I created two pre-configured jobs that select all assets excluding the hourly_emissions_epacems and downstream assets.

I also made some changes in the pudl_etl cli command so users can specify a settings file without epacems. This feels a little janky because the epacems settings are still floating around in the dataset_settings resource even though the assets that depend on epacems settings aren't being used. At some point, it might make sense to remove the dataset_settings resource and just configure the extraction assets directly.

PR Checklist

  • Merge the most recent version of the branch you are merging into (probably dev).
  • All CI checks are passing. Run tests locally to debug failures
  • Make sure you've included good docstrings.
  • For major data coverage & analysis changes, run data validation tests
  • Include unit tests for new functions and classes.
  • Defensive data quality/sanity checks in analyses & data processing functions.
  • Update the release notes and reference reference the PR and related issues.
  • Do your own explanatory review of the PR to help the reviewer understand what's going on and identify issues preemptively.

@bendnorman bendnorman requested a review from zschira February 28, 2023 23:59
@codecov
Copy link

codecov bot commented Mar 1, 2023

Codecov Report

❗ No coverage uploaded for pull request base (dagster-asset-etl@deb2d51). Click here to learn what that means.
Patch coverage: 58.8% of modified lines in pull request are covered.

❗ Current head 52a8bbe differs from pull request most recent head 2e10526. Consider uploading reports for the commit 2e10526 to get more accurate results

Additional details and impacted files
@@                 Coverage Diff                 @@
##             dagster-asset-etl   #2343   +/-   ##
===================================================
  Coverage                     ?   85.8%           
===================================================
  Files                        ?      80           
  Lines                        ?    9497           
  Branches                     ?       0           
===================================================
  Hits                         ?    8154           
  Misses                       ?    1343           
  Partials                     ?       0           
Impacted Files Coverage Δ
src/pudl/workspace/setup.py 83.8% <ø> (ø)
src/pudl/cli.py 58.9% <25.0%> (ø)
src/pudl/helpers.py 86.1% <88.8%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@zschira zschira left a comment

Choose a reason for hiding this comment

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

Looks good!

@bendnorman bendnorman merged commit 15b3196 into dagster-asset-etl Mar 3, 2023
@bendnorman bendnorman deleted the dagster-epacems-selection branch March 3, 2023 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants