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

Modernize importlib usage #2759

Merged
merged 3 commits into from
Jul 29, 2023
Merged

Modernize importlib usage #2759

merged 3 commits into from
Jul 29, 2023

Conversation

zaneselvans
Copy link
Member

@zaneselvans zaneselvans commented Jul 28, 2023

PR Overview

  • Updates our usage of importlib.resources to use the current API in a uniform and relatively simple way across the repo. Previously we had a mix of old deprecated functions and an unnecessarily complex syntax from the new API. In almost all cases we can simply use the Traversable class and its pathlib.Path like interface.
  • Also avoids use of the deprecated pkg_resources module which is going to be removed from the standard library in Python 3.12, replacing it with functionality from importlib.metadata

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that nearly everything in this documentation page is outdated, since we've moved to using pyproject.toml exclusively to define our packaging.

Copy link
Member

Choose a reason for hiding this comment

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

Good point, I think we can just rip out the whole setup.py section. I feel like we're not doing anything so outré with the pyproject.toml that we need special documentation on how to use it, TBH.

@@ -300,7 +300,6 @@ doctest_optionflags = NORMALIZE_WHITESPACE IGNORE_EXCEPTION_DETAIL ELLIPSIS
filterwarnings =
ignore:distutils Version classes are deprecated:DeprecationWarning
Copy link
Member Author

Choose a reason for hiding this comment

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

Unsure of why this warning is still coming up. Maybe from an imported package? I don't think we're directly using any of the pkg_resources versions now.

Copy link
Member

Choose a reason for hiding this comment

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

I poked around a bit and saw some old pkg_resources deprecation warnings from the google SDK libaries.

@zaneselvans zaneselvans changed the title Modernize importlib Modernize importlib usage Jul 28, 2023
@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

Patch coverage: 76.0% and no project coverage change.

Comparison is base (e9735a2) 88.4% compared to head (100f87f) 88.4%.
Report is 4 commits behind head on dev.

Additional details and impacted files
@@          Coverage Diff          @@
##             dev   #2759   +/-   ##
=====================================
  Coverage   88.4%   88.4%           
=====================================
  Files         89      89           
  Lines      10199   10192    -7     
=====================================
- Hits        9024    9018    -6     
+ Misses      1175    1174    -1     
Files Changed Coverage Δ
src/pudl/analysis/ferc1_eia_train.py 52.9% <0.0%> (-0.4%) ⬇️
src/pudl/transform/eia.py 97.2% <ø> (ø)
src/pudl/transform/ferc1.py 96.8% <50.0%> (+<0.1%) ⬆️
src/pudl/workspace/setup.py 84.1% <50.0%> (+2.5%) ⬆️
src/pudl/analysis/ferc1_eia.py 98.5% <100.0%> (-0.1%) ⬇️
src/pudl/extract/dbf.py 89.4% <100.0%> (ø)
src/pudl/extract/excel.py 94.6% <100.0%> (-0.1%) ⬇️
src/pudl/glue/ferc1_eia.py 98.4% <100.0%> (ø)
src/pudl/helpers.py 87.0% <100.0%> (ø)
src/pudl/output/sql/helpers.py 100.0% <100.0%> (ø)

... and 1 file with indirect coverage changes

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

Copy link
Member

@jdangerx jdangerx left a comment

Choose a reason for hiding this comment

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

This is nice, thanks for cleaning it up! I'm ok with the codecov patch coverage warning.

)
with importlib.resources.as_file(pkg_source) as yf:
etl_fast_settings = EtlSettings.from_yaml(yf).datasets
etl_fast_settings = EtlSettings.from_yaml(
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

Copy link
Member

Choose a reason for hiding this comment

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

Good point, I think we can just rip out the whole setup.py section. I feel like we're not doing anything so outré with the pyproject.toml that we need special documentation on how to use it, TBH.

@@ -300,7 +300,6 @@ doctest_optionflags = NORMALIZE_WHITESPACE IGNORE_EXCEPTION_DETAIL ELLIPSIS
filterwarnings =
ignore:distutils Version classes are deprecated:DeprecationWarning
Copy link
Member

Choose a reason for hiding this comment

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

I poked around a bit and saw some old pkg_resources deprecation warnings from the google SDK libaries.

@zaneselvans zaneselvans merged commit f11275a into dev Jul 29, 2023
9 of 10 checks passed
@zaneselvans zaneselvans deleted the modernize-importlib branch September 12, 2023 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants