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

Replace deprecated pkg_resources #3335

Merged
merged 10 commits into from
Sep 22, 2023

Conversation

kratman
Copy link
Contributor

@kratman kratman commented Sep 14, 2023

Description

While looking through the code, I noticed a deprecation warning for pkg_resources. I replaced the two usages I found in the code with importlib.metadata.

Type of change

Only changes an imported library, there should be no real changes to the functionality.

Key checklist:

  • No style issues: $ pre-commit run (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
  • All tests pass: $ python run-tests.py --all (or $ nox -s tests)
  • The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s quick).

Further checks:

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@kratman kratman marked this pull request as draft September 14, 2023 17:40
@kratman kratman marked this pull request as ready for review September 14, 2023 18:03
@codecov
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.02% ⚠️

Comparison is base (8af2430) 99.57% compared to head (31c362d) 99.56%.
Report is 30 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3335      +/-   ##
===========================================
- Coverage    99.57%   99.56%   -0.02%     
===========================================
  Files          253      253              
  Lines        19571    19559      -12     
===========================================
- Hits         19488    19474      -14     
- Misses          83       85       +2     
Files Changed Coverage Δ
pybamm/parameters/parameter_sets.py 100.00% <100.00%> (ø)
pybamm/util.py 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

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

@kratman
Copy link
Contributor Author

kratman commented Sep 14, 2023

The drop in coverage appears to be from the code in #3116, not the changes from this PR

@kratman kratman force-pushed the feat/replacePkgResources branch from f6641c9 to 07f7068 Compare September 14, 2023 21:23
@agriyakhetarpal
Copy link
Member

Thanks for this @kratman. I have been doing the same thing in #3301 😄

@kratman
Copy link
Contributor Author

kratman commented Sep 15, 2023

@agriyakhetarpal I did not see the changes there when I went to do this. This one is small and ready to go though.

I rebased yesterday to see if it would retrigger the coverage report which was showing a drop due to a different merged PR. It passed everything else before I did the rebase.

@agriyakhetarpal
Copy link
Member

Yes, that PR is a bit stalled though, for other reasons. The changes there are similar to yours and everything passes, I expect them to pass here too

@kratman
Copy link
Contributor Author

kratman commented Sep 15, 2023

The failing tests (nox -s examples) for this PR are failing on develop as well.

@kratman kratman changed the base branch from develop to 1+1D-thermal-bug September 19, 2023 13:20
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@kratman kratman changed the base branch from 1+1D-thermal-bug to develop September 19, 2023 13:20
pybamm/parameters/parameter_sets.py Outdated Show resolved Hide resolved
pybamm/parameters/parameter_sets.py Show resolved Hide resolved
Copy link
Member

@agriyakhetarpal agriyakhetarpal 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 to me, thanks! @Saransh-cpp, could you trigger a workflow run here?

@kratman
Copy link
Contributor Author

kratman commented Sep 21, 2023

@Saransh-cpp, @agriyakhetarpal It looks like coverage is dropping due to the version logic. Is there something else you want me to change, or is this OK?

@agriyakhetarpal
Copy link
Member

Perhaps adding # pragma: no cover on the conditional should be okay in this case

@Saransh-cpp
Copy link
Member

Perhaps adding # pragma: no cover on the conditional should be okay in this case

Yes, should be fine in this case.

@kratman
Copy link
Contributor Author

kratman commented Sep 22, 2023

Changed thanks

Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

Thanks, @kratman!

@Saransh-cpp Saransh-cpp merged commit 3c48029 into pybamm-team:develop Sep 22, 2023
29 of 32 checks passed
@kratman kratman deleted the feat/replacePkgResources branch September 22, 2023 22:00
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.

3 participants