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

Run ferc_to_sqlite and pudl_etl independent of integration tests #2825

Open
wants to merge 113 commits into
base: main
Choose a base branch
from

Conversation

rousik
Copy link
Collaborator

@rousik rousik commented Sep 3, 2023

This reworks how tests (unit, integration) are done by github actions.

In particular, it modifies how ci-integration is executed by running ferc_to_sqlite and pudl_etl commands independently, making use of dagster parallelism and larger runners.

@rousik rousik changed the base branch from main to dev September 3, 2023 16:35
@rousik rousik changed the title Parallel pytest - PR to test the new flow Run pudl_etl prior to running integration tests Sep 6, 2023
@zaneselvans
Copy link
Member

I think the call to pytest is failing because you need to install some or all of the extra dependencies when you install the catalystcoop.pudl package -- right now you're just installing the bare minimum requirements. so...

pip install ./[doc,test,dev,datasette]

rather than just

pip install .

@rousik
Copy link
Collaborator Author

rousik commented Dec 13, 2023

I think I'm going to loose my mind here. I thought I finally figured the right configuration here only to find out that it still seems to be busted, so back to the rabbit hole of investigations 🤦

@zaneselvans
Copy link
Member

zaneselvans commented Dec 15, 2023

@rousik I merged dev in to get the pre-commit version of the lockfile updates, which should involve much less churn.

It seems like it's mostly getting the coverage, with 90.1%. Are the tests themselves not being covered now maybe? It looks like somehow the test/unit and test/integration lines in the coverage config section of pyproject.toml got lost merging. I added them back in.

@zaneselvans zaneselvans added the testing Writing tests, creating test data, automating testing, etc. label Dec 15, 2023
Base automatically changed from dev to main January 5, 2024 04:14
@zaneselvans
Copy link
Member

@rousik Can you update the main description for this PR to reflect the current situation? It's been through so many changes and the code on main has changed so much that I'm no longer clear what the net effect is.

The integration test run time seems to be almost identical to the current tests (~1.5 hours).

Is the primary change that this PR is running the ETL outside the context of pytest? Where were we going with that? Is the next step to kick off the FERC to SQLite conversions as their own jobs on smaller runners which can run in parallel rather than serially, and have the main PUDL ETL start on a larger runner as soon as the FERC 1 DBs are available, hopefully shortening the overall integration test runtime and also reducing the running time for the paid runner?

@rousik
Copy link
Collaborator Author

rousik commented Jan 18, 2024

@rousik Can you update the main description for this PR to reflect the current situation? It's been through so many changes and the code on main has changed so much that I'm no longer clear what the net effect is.

The integration test run time seems to be almost identical to the current tests (~1.5 hours).

Is the primary change that this PR is running the ETL outside the context of pytest? Where were we going with that? Is the next step to kick off the FERC to SQLite conversions as their own jobs on smaller runners which can run in parallel rather than serially, and have the main PUDL ETL start on a larger runner as soon as the FERC 1 DBs are available, hopefully shortening the overall integration test runtime and also reducing the running time for the paid runner?

I suppose I need to rerun some of these tests again, because we should be getting speed-ups from this even if nothing more is done. Isolating ferc_to_sqlite, pudl_etl and integration tests in separate steps should make it a little more easy to diagnose which parts failed, and should also (as pointed out) allow us to further speed-up/improve the individual steps as needed. We could beef-up the pudl_etl part, hopefully getting faster there, and then sharding ferc_to_sqlite which should be a good time improvement.

@rousik
Copy link
Collaborator Author

rousik commented Jan 18, 2024

@rousik Can you update the main description for this PR to reflect the current situation? It's been through so many changes and the code on main has changed so much that I'm no longer clear what the net effect is.

The integration test run time seems to be almost identical to the current tests (~1.5 hours).

Is the primary change that this PR is running the ETL outside the context of pytest? Where were we going with that? Is the next step to kick off the FERC to SQLite conversions as their own jobs on smaller runners which can run in parallel rather than serially, and have the main PUDL ETL start on a larger runner as soon as the FERC 1 DBs are available, hopefully shortening the overall integration test runtime and also reducing the running time for the paid runner?

I have re-read contents of this PR and description and I think they are still in sync. This refactors ci-integration into smaller individual tasks.

Copy link
Member

@zaneselvans zaneselvans left a comment

Choose a reason for hiding this comment

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

Mostly requests for clarification on why some things are changing.

.github/workflows/pytest.yml Show resolved Hide resolved
.github/workflows/pytest.yml Show resolved Hide resolved
Comment on lines -115 to -116
strategy:
fail-fast: false
Copy link
Member

Choose a reason for hiding this comment

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

Why do we want fail-fast enabled, given that we've already the dependency above specified? I think this would mean that if the docs build fails, the unit and integration tests would be canceled, right?

.github/workflows/pytest.yml Show resolved Hide resolved
.github/workflows/pytest.yml Show resolved Hide resolved
@@ -198,37 +221,40 @@ jobs:
with:
path: coverage
Copy link
Member

Choose a reason for hiding this comment

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

Why do we use path: coverage here but when we're uploading the artifact above we use path: coverage.xml?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps this is a bit of confusing api. When uploading artifacts, path tells you what files/directories you want to upload, and optionally you can give them name to uniquely idenfiy them. When you're downloading artifacts, path tells you where to download things. You can either pick specific artifact names to download, or you can just download everything (which is happening here). What happens is that you'll end up with coverage/coverage-unit/coverage.xml, coverage/coverage-integration/coverage.xml and so on.

pyproject.toml Outdated
Comment on lines 312 to 319
# See note above on need to specify separate sources for pytest-coverage and coverage.
source = ["src/pudl/", "test/integration/", "test/unit/"]
# source = ["src/pudl/", "test/integration/", "test/unit/"]
include = [
"src/pudl/**",
"test/integration/**",
"test/unit/**",
"*/site-packages/pudl/**",
]
Copy link
Member

Choose a reason for hiding this comment

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

What's going on with the change from source to include here? Should these be identical to the pudl_sources below? Should the commented out source line be removed? Is the comment referring above still relevant? Does switching from source to include mean we don't need the --cov=... arguments above?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we are not installing as editable, pudl will be loaded from site-packages/pudl and I feel like source was misbehaving and did not quite work, while include does.

AFAIK in the new ci-integration that is part of this PR, the coverage commands should work w/o the need of --cov arguments above. I'm not even sure that was there when I was working on this PR initially.

pudl_sources below is a translation block that maps site-packages/pudl/foo.py onto src/pudl/foo.py, so that the coverage works even when the libraries are loaded form slightly different location.

I suppose all of this could be made to work if we simply install pudl as editable.

.github/workflows/pytest.yml Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@rousik rousik left a comment

Choose a reason for hiding this comment

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

Huh, lots of my comments were left in limbo. Posting.

.github/workflows/pytest.yml Outdated Show resolved Hide resolved
@@ -58,13 +64,13 @@ jobs:
path: coverage.xml

ci-unit:
name: Unit tests
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you know where are those merge requirements specified? I think it would be nice to use human friendly names here, but we can do it in a separate step distinct from this one.

.github/workflows/pytest.yml Outdated Show resolved Hide resolved
.github/workflows/pytest.yml Show resolved Hide resolved
.github/workflows/pytest.yml Show resolved Hide resolved
.github/workflows/pytest.yml Show resolved Hide resolved
.github/workflows/pytest.yml Show resolved Hide resolved
@@ -198,37 +221,40 @@ jobs:
with:
path: coverage
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps this is a bit of confusing api. When uploading artifacts, path tells you what files/directories you want to upload, and optionally you can give them name to uniquely idenfiy them. When you're downloading artifacts, path tells you where to download things. You can either pick specific artifact names to download, or you can just download everything (which is happening here). What happens is that you'll end up with coverage/coverage-unit/coverage.xml, coverage/coverage-integration/coverage.xml and so on.

.github/workflows/pytest.yml Show resolved Hide resolved
pyproject.toml Outdated
Comment on lines 312 to 319
# See note above on need to specify separate sources for pytest-coverage and coverage.
source = ["src/pudl/", "test/integration/", "test/unit/"]
# source = ["src/pudl/", "test/integration/", "test/unit/"]
include = [
"src/pudl/**",
"test/integration/**",
"test/unit/**",
"*/site-packages/pudl/**",
]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we are not installing as editable, pudl will be loaded from site-packages/pudl and I feel like source was misbehaving and did not quite work, while include does.

AFAIK in the new ci-integration that is part of this PR, the coverage commands should work w/o the need of --cov arguments above. I'm not even sure that was there when I was working on this PR initially.

pudl_sources below is a translation block that maps site-packages/pudl/foo.py onto src/pudl/foo.py, so that the coverage works even when the libraries are loaded form slightly different location.

I suppose all of this could be made to work if we simply install pudl as editable.

@zaneselvans zaneselvans changed the title Rework integration tests (more parallelism, nicer things) Run ferc_to_sqlite and pudl_etl independent of integration tests Jan 19, 2024
Copy link
Member

@zaneselvans zaneselvans left a comment

Choose a reason for hiding this comment

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

Comparing locally generated test coverage and the CodeCov output from CI.

Comment on lines -313 to +336
source = ["src/pudl/", "test/integration/", "test/unit/"]
include = [
"src/pudl/**",
"test/integration/**",
"test/unit/**",
"*/site-packages/pudl/**",
]
omit = [
# Never hit by integration tests:
"src/pudl/validate.py",
]
sigterm = true
concurrency=["multiprocessing"]
debug = ["config", "trace"]

[tool.coverage.paths]
# When running pudl tools installed with pip, the sources are imported
# from package-data/pudl directory. The following maps this to raw
# source files.
pudl_sources = [
"src/pudl/",
"*/site-packages/pudl/",
"test/unit",
"test/integration",
]
Copy link
Member

Choose a reason for hiding this comment

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

I'm re-running the CodeCov tests now, but it seems like something has changed that's reducing the test coverage by a few hundred lines / a couple of percent, and I don't understand what it is.

Looking at the indirect coverage changes some differences make sense, like increased coverage for the ferc_to_sqlite and pudl_etl CLIs, but there's also substantial apparent losses of coverage in modules related to the FERC-EIA record linkage, and the pudltabl output module which don't make sense.

If I run

make pytest-coverage

locally I get overall coverage of 92.8% which is comparable to the coverage on main

Name                                                           Stmts   Miss  Cover
----------------------------------------------------------------------------------
src/pudl/__init__.py                                              14      0 100.0%
src/pudl/analysis/__init__.py                                      1      0 100.0%
src/pudl/analysis/fuel_by_plant.py                                37      0 100.0%
src/pudl/analysis/record_linkage/__init__.py                       1      0 100.0%
src/pudl/convert/__init__.py                                       1      0 100.0%
src/pudl/etl/__init__.py                                          22      0 100.0%
src/pudl/etl/eia_bulk_elec_assets.py                               9      0 100.0%
src/pudl/etl/epacems_assets.py                                    54      0 100.0%
src/pudl/etl/static_assets.py                                     24      0 100.0%
src/pudl/extract/__init__.py                                       1      0 100.0%
src/pudl/extract/csv.py                                           29      0 100.0%
src/pudl/extract/eia176.py                                         9      0 100.0%
src/pudl/extract/eia860.py                                        40      0 100.0%
src/pudl/extract/eia860m.py                                       30      0 100.0%
src/pudl/extract/eia923.py                                        51      0 100.0%
src/pudl/extract/eia_bulk_elec.py                                 46      0 100.0%
src/pudl/extract/epacems.py                                       47      0 100.0%
src/pudl/extract/ferc.py                                          14      0 100.0%
src/pudl/extract/ferc2.py                                         21      0 100.0%
src/pudl/extract/ferc6.py                                         15      0 100.0%
src/pudl/extract/ferc60.py                                        15      0 100.0%
src/pudl/extract/ferc714.py                                       21      0 100.0%
src/pudl/ferc_to_sqlite/__init__.py                               24      0 100.0%
src/pudl/glue/__init__.py                                          1      0 100.0%
src/pudl/metadata/__init__.py                                      1      0 100.0%
src/pudl/metadata/codes.py                                         5      0 100.0%
src/pudl/metadata/constants.py                                    21      0 100.0%
src/pudl/metadata/dfs.py                                          12      0 100.0%
src/pudl/metadata/enums.py                                        21      0 100.0%
src/pudl/metadata/fields.py                                       25      0 100.0%
src/pudl/metadata/labels.py                                       12      0 100.0%
src/pudl/metadata/resources/__init__.py                           12      0 100.0%
src/pudl/metadata/resources/allocate_gen_fuel.py                   4      0 100.0%
src/pudl/metadata/resources/eia.py                                 4      0 100.0%
src/pudl/metadata/resources/eia860.py                              3      0 100.0%
src/pudl/metadata/resources/eia861.py                              2      0 100.0%
src/pudl/metadata/resources/eia923.py                              4      0 100.0%
src/pudl/metadata/resources/eia_bulk_elec.py                       2      0 100.0%
src/pudl/metadata/resources/epacems.py                             3      0 100.0%
src/pudl/metadata/resources/ferc1.py                               4      0 100.0%
src/pudl/metadata/resources/ferc1_eia_record_linkage.py            3      0 100.0%
src/pudl/metadata/resources/ferc714.py                             3      0 100.0%
src/pudl/metadata/resources/glue.py                                3      0 100.0%
src/pudl/metadata/resources/mcoe.py                                4      0 100.0%
src/pudl/metadata/resources/pudl.py                                3      0 100.0%
src/pudl/metadata/sources.py                                       5      0 100.0%
src/pudl/output/__init__.py                                        1      0 100.0%
src/pudl/output/eia860.py                                         19      0 100.0%
src/pudl/output/eia_bulk_elec.py                                  14      0 100.0%
src/pudl/output/sql/helpers.py                                    12      0 100.0%
src/pudl/resources.py                                             17      0 100.0%
src/pudl/transform/__init__.py                                     1      0 100.0%
src/pudl/transform/eia_bulk_elec.py                               26      0 100.0%
src/pudl/transform/ferc714.py                                     76      0 100.0%
src/pudl/transform/params/__init__.py                              1      0 100.0%
src/pudl/transform/params/ferc1.py                                67      0 100.0%
src/pudl/workspace/__init__.py                                     1      0 100.0%
test/integration/console_scripts_test.py                          23      0 100.0%
test/integration/datasette_metadata_test.py                       22      0 100.0%
test/integration/etl_test.py                                      76      0 100.0%
test/integration/ferc1_eia_train_test.py                          30      0 100.0%
test/integration/ferc_dbf_extract_test.py                         38      0 100.0%
test/integration/zenodo_datapackage_test.py                       11      0 100.0%
test/unit/analysis/plant_parts_eia_test.py                        48      0 100.0%
test/unit/analysis/spatial_test.py                                67      0 100.0%
test/unit/analysis/state_demand_test.py                            8      0 100.0%
test/unit/analysis/timeseries_cleaning_test.py                    32      0 100.0%
test/unit/console_scripts_test.py                                 10      0 100.0%
test/unit/extract/csv_test.py                                     37      0 100.0%
test/unit/extract/eia_bulk_elec_test.py                           42      0 100.0%
test/unit/extract/xbrl_test.py                                    74      0 100.0%
test/unit/helpers_test.py                                        135      0 100.0%
test/unit/output/epacems_test.py                                   7      0 100.0%
test/unit/output/ferc1_test.py                                     9      0 100.0%
test/unit/transform/classes_test.py                              218      0 100.0%
test/unit/transform/eia923_test.py                                12      0 100.0%
test/unit/transform/eia_bulk_elec_test.py                         18      0 100.0%
test/unit/transform/epacems_test.py                                8      0 100.0%
test/unit/transform/ferc1_test.py                                133      0 100.0%
test/unit/transform/glue_test.py                                  11      0 100.0%
test/unit/workspace/datastore_test.py                             82      0 100.0%
test/unit/workspace/resource_cache_test.py                       140      0 100.0%
src/pudl/extract/eia861.py                                        36      1  97.2%
src/pudl/extract/ferc1.py                                        113      1  99.1%
test/integration/epacems_test.py                                  39      1  97.4%
test/integration/output_test.py                                   69      1  98.6%
test/unit/settings_test.py                                       145      1  99.3%
src/pudl/__main__.py                                               2      2   0.0%
src/pudl/analysis/mcoe.py                                         78      2  97.4%
src/pudl/analysis/record_linkage/classify_plants_ferc1.py         31      2  93.5%
src/pudl/etl/glue_assets.py                                      112      2  98.2%
src/pudl/glue/ferc1_eia.py                                       135      2  98.5%
src/pudl/output/censusdp1tract.py                                 28      2  92.9%
src/pudl/output/eia923.py                                         93      2  97.8%
src/pudl/workspace/setup.py                                       43      2  95.3%
test/unit/analysis/allocate_gen_fuel_test.py                      96      2  97.9%
test/unit/extract/excel_test.py                                   49      2  95.9%
test/unit/harvest_test.py                                         59      2  96.6%
test/unit/metadata_test.py                                        56      2  96.4%
src/pudl/extract/xbrl.py                                          52      3  94.2%
src/pudl/logging_helpers.py                                       15      3  80.0%
src/pudl/output/epacems.py                                        29      3  89.7%
src/pudl/settings.py                                             253      3  98.8%
src/pudl/transform/epacems.py                                     34      3  91.2%
test/integration/record_linkage_test.py                           72      3  95.8%
src/pudl/convert/censusdp1tract_to_sqlite.py                      35      4  88.6%
src/pudl/metadata/helpers.py                                     187      4  97.9%
test/integration/glue_test.py                                     42      4  90.5%
test/unit/glue.py                                                  7      4  42.9%
src/pudl/output/ferc714.py                                       150      5  96.7%
test/integration/jupyter_notebooks_test.py                        13      5  61.5%
src/pudl/analysis/record_linkage/embed_dataframe.py              124      6  95.2%
src/pudl/analysis/record_linkage/eia_ferc1_record_linkage.py     218      7  96.8%
src/pudl/convert/metadata_to_rst.py                               21      7  66.7%
src/pudl/analysis/spatial.py                                     112      8  92.9%
src/pudl/etl/check_foreign_keys.py                                20      8  60.0%
src/pudl/extract/excel.py                                        167      8  95.2%
src/pudl/transform/eia.py                                        300      8  97.3%
src/pudl/analysis/plant_parts_eia.py                             260      9  96.5%
src/pudl/workspace/resource_cache.py                             124      9  92.7%
src/pudl/transform/eia861.py                                     398     10  97.5%
test/unit/io_managers_test.py                                    206     10  95.1%
src/pudl/output/pudltabl.py                                       98     11  88.8%
src/pudl/analysis/epacamd_eia.py                                  18     12  33.3%
src/pudl/analysis/record_linkage/link_cross_year.py              106     12  88.7%
src/pudl/analysis/record_linkage/name_cleaner.py                  90     12  86.7%
src/pudl/analysis/allocate_gen_fuel.py                           323     13  96.0%
src/pudl/analysis/state_demand.py                                146     15  89.7%
src/pudl/ferc_to_sqlite/cli.py                                    44     15  65.9%
src/pudl/transform/classes.py                                    394     21  94.7%
src/pudl/etl/cli.py                                               48     22  54.2%
src/pudl/transform/eia860.py                                     205     23  88.8%
src/pudl/extract/dbf.py                                          258     25  90.3%
src/pudl/workspace/datastore.py                                  298     25  91.6%
src/pudl/io_managers.py                                          268     26  90.3%
src/pudl/analysis/service_territory.py                           123     31  74.8%
src/pudl/transform/eia923.py                                     231     34  85.3%
src/pudl/analysis/timeseries_cleaning.py                         460     44  90.4%
src/pudl/helpers.py                                              407     53  87.0%
src/pudl/output/ferc1.py                                         632     66  89.6%
src/pudl/transform/ferc1.py                                     1545     68  95.6%
src/pudl/output/eia.py                                           176     72  59.1%
src/pudl/analysis/record_linkage/eia_ferc1_train.py              172     79  54.1%
src/pudl/metadata/classes.py                                     798    101  87.3%
----------------------------------------------------------------------------------
TOTAL                                                          13072    943  92.8%
  • test/unit/glue.py needs to be renamed so pytest collects tests from it, but that's not a new thing.
  • test/integration/console_scripts_test.py doesn't get hit all in the CodeCov report but has 100% coverage if I run locally.
  • src/pudl/analysis/record_linkage/eia_ferc1_train.py coverage is dramatically lower in CodeCov than locally.
  • src/pudl/analysis/service_territory.py is also significantly lower. My guess is this is related to the scripts not getting tested?
  • src/pudl/output/pudltabl.py is basically not getting hit at all in CodeCov which makes no sense to me, since the PudlTabl objects are used all over the place in the tests and have 88.8% coverage when run locally.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry for delays, life has been busy. I will try to follow up on these in the coming week and try to figure out what is going on here. The fact that coverage is so brittle and it is so hard to figure out why it fluctuates so much is very discouraging; it shouldn't be that hard :-/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community testing Writing tests, creating test data, automating testing, etc.
Projects
Status: Icebox
Development

Successfully merging this pull request may close these issues.

3 participants