-
Notifications
You must be signed in to change notification settings - Fork 27
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
Replace deprecated pkg_resources #667
base: main
Are you sure you want to change the base?
Conversation
@@ -30,7 +30,8 @@ | |||
"imageio-ffmpeg", | |||
"imageio", | |||
"tqdm", | |||
"epiweeks" | |||
"epiweeks", | |||
"importlib_resources==6.1.1", |
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.
Should we be pinning the exact version? I'd assume we just want a compatible version, as in importlib_resources~=6.1
, or something like that. Or is there a reason we need this exact version?
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.
It definitely makes sense for this to less rigid than requiring an exact version, particularly since the other packages are specified with open-ended constraints (or none at all). Based on the last comment linked gitlab issue, it looks like this could be:
"importlib_resources==6.1.1", | |
"importlib_resources>=1.3", |
Backports are nice for functionality but potentially headache-inducing w.r.t. dependency complexity... We could make the requirement "importlib_resources>=1.3;python_version<"3.9"
" but that requires the code to know when it should use importlib_resources
vs importlib.resources
pkg_resources.resource_filename(__name__, "geo_mappings/state_census.csv"), dtype=str) | ||
import importlib_resources | ||
|
||
county_census = importlib_resources.files(__name__) / 'geo_mappings' / 'county_census.csv' |
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.
since this is the top-level namespace for this sub-package, we should either prefix this variable name with an underscore or avoid creating the variable in the first place. this is especially true because its name is the lowercase version of the name of a constant, so maybe make the name more descriptive if you want to keep it around.
county_census = importlib_resources.files(__name__) / 'geo_mappings' / 'county_census.csv' | |
_county_census_file_path = importlib_resources.files(__name__) / 'geo_mappings' / 'county_census.csv' |
import importlib_resources | ||
|
||
county_census = importlib_resources.files(__name__) / 'geo_mappings' / 'county_census.csv' | ||
with importlib_resources.as_file(county_census) as path: |
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.
does this context manager afford us anything? it looks like pd.read_csv()
should be able to accept the county_census
variable directly.
Closes #666.
Summary:
Removes uses of setuptools' deprecated
pkg_resources
library - these are replaced by the nativeimportlib.metadata
library, but also theimportlib_resources
backport. This is due to the features needed in the codebase, like accessing resources within subdirectories, only becoming available in Python 3.9+.Prerequisites:
dev
branchdev