-
Notifications
You must be signed in to change notification settings - Fork 115
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 pkg_resource with importlib.metadata #2778
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2778 +/- ##
=======================================
Coverage 67.14% 67.15%
=======================================
Files 561 561
Lines 104145 104145
=======================================
+ Hits 69930 69935 +5
+ Misses 34215 34210 -5 ☔ View full report in Codecov by Sentry. |
@anilbey : just a heads-up, you can see that Azure CI where we build wheels is failing. I didn't check and don't have enough knowledge about this changeset / pkg_resource. But in case it's relevant, I want to mention that we build wheels with two names: |
|
||
package_name = "neuron" | ||
# Search for NEURON package installation location using importlib.metadata | ||
for dist in distributions(): |
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.
I think it's a bit clearer to use metadata("neuron-nightly")
in a try..except PackageNotFoundError
here as we did for NMODL.
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.
Hi @JCGoran, ah great so you already did this change for NMODL.
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.
I actually didn't understand this part. I don't know how the code should behave for neuron-nightly. Do you mind adding this change here?
@@ -7,7 +7,7 @@ | |||
import shutil | |||
import subprocess | |||
import sys | |||
from pkg_resources import working_set | |||
from importlib.metadata import distributions |
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.
importlib.metadata
is only present in Python 3.9 and above; since I see NEURON nightly on PyPI still has a 3.8 version available for download, you can use the backported importlib_metadata
third-party module for it.
Note that then we also need to update the NEURON recipe for Spack (I think it's this file).
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.
Thanks I added the backport package.
|
Superseded by #2820 |
To address the following deprecation.