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

Remove pkg resources #1039

Merged
merged 6 commits into from
Apr 11, 2024
Merged

Remove pkg resources #1039

merged 6 commits into from
Apr 11, 2024

Conversation

ocefpaf
Copy link
Member

@ocefpaf ocefpaf commented Aug 1, 2023

pkg_resources is notoriously slow and problematic. This PR tries to remove it from compliance-checker.

@codecov
Copy link

codecov bot commented Aug 9, 2023

Codecov Report

Attention: Patch coverage is 88.88889% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 82.18%. Comparing base (c137e28) to head (d3ee89b).
Report is 7 commits behind head on develop.

Files Patch % Lines
compliance_checker/cf/util.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1039      +/-   ##
===========================================
+ Coverage    82.14%   82.18%   +0.04%     
===========================================
  Files           24       24              
  Lines         5170     5171       +1     
  Branches      1242     1242              
===========================================
+ Hits          4247     4250       +3     
+ Misses         623      622       -1     
+ Partials       300      299       -1     

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

@ocefpaf ocefpaf marked this pull request as ready for review August 9, 2023 14:16
@ocefpaf
Copy link
Member Author

ocefpaf commented Aug 9, 2023

@benjwadams there is still one part of pkg_resources that we in the plugins stuff, the pkg_resources.working_set, but that one is a bit tricky and I'll leave it to another PR. This one is ready to review.

@ocefpaf ocefpaf requested a review from benjwadams August 9, 2023 14:18
@ocefpaf ocefpaf force-pushed the remove_pkg_resources branch 2 times, most recently from b2c0a29 to 726b120 Compare September 5, 2023 18:01
@ocefpaf
Copy link
Member Author

ocefpaf commented Sep 5, 2023

@benjwadams this is hurting me in a setup where I have tons of packages and pkg_resources really drags everything down. While we still have the plugins to deal with, for now, this already helps speed things up. Do you mind taking a look?

@ocefpaf ocefpaf force-pushed the remove_pkg_resources branch from 726b120 to 295a33b Compare October 4, 2023 15:18
@benjwadams
Copy link
Contributor

Hi, sorry for the delay. Will have a look.

@benjwadams
Copy link
Contributor

benjwadams commented Oct 17, 2023

This should still work with Compliance Checker plugins, correct?

@benjwadams
Copy link
Contributor

Just did a test in a pristine environment with cc-plugin-glider and it worked. However, when installing via pip, it looks like packaging package is missing, so should probably be added to requirements.txt.

@ocefpaf
Copy link
Member Author

ocefpaf commented Oct 17, 2023

Just did a test in a pristine environment with cc-plugin-glider and it worked.

Cool. The plugin tests are also passing in the CIs. We do need to review the whole plugin system at some point though. But not now 😬

However, when installing via pip, it looks like packaging package is missing, so should probably be added to requirements.txt.

Done! I guess some other dependency is pulling it in the CIs b/c it did not fail there.

@ocefpaf
Copy link
Member Author

ocefpaf commented Oct 18, 2023

The glider and ugrid plugin failures are an issue upstream I'll try to tackle them this week.

@ocefpaf
Copy link
Member Author

ocefpaf commented Oct 19, 2023

With ioos/cc-plugin-glider#42 and ioos/cc-plugin-ugrid#19 this one is green again!

@jcermauwedu jcermauwedu mentioned this pull request Mar 19, 2024
@jcermauwedu
Copy link

What is the status of trying to include this PR? I look forward to the reduced warnings generated by the test suite.

@ocefpaf
Copy link
Member Author

ocefpaf commented Apr 3, 2024

What is the status of trying to include this PR? I look forward to the reduced warnings generated by the test suite.

I just rebased it to make it mergeable again.

@benjwadams do you want us to do more testing or are you good with this PR?

@ChrisBarker-NOAA
Copy link
Contributor

I just duplicated this work in: #1059 -- not sure if I did a better or worse job. But it would be great if one or the other were merged.

@jcermauwedu
Copy link

@ChrisBarker-NOAA @ocefpaf @benjwadams I know, I almost did it a 3rd time with this PR(#1056) to fix some of the python tests. I had to undo those fixes when I remembered they were in this PR.

What can we do to help?

@ChrisBarker-NOAA
Copy link
Contributor

What's the blocker to merging this now? Probably better than mine, as mine has a few checks failing -- might as well do this first, and then rebase mine and figure out what's up.

@ocefpaf
Copy link
Member Author

ocefpaf commented Apr 10, 2024

What's the blocker to merging this now? Probably better than mine, as mine has a few checks failing -- might as well do this first, and then rebase mine and figure out what's up.

I'm waiting for Ben's response today and, if he is to busy, I'll self-merge this one. Stay tuned...

@ChrisBarker-NOAA
Copy link
Contributor

great, thanks!

-CHB

@ocefpaf ocefpaf force-pushed the remove_pkg_resources branch from 9f99a53 to d3ee89b Compare April 11, 2024 09:13
@ocefpaf ocefpaf merged commit ee4f7b6 into ioos:develop Apr 11, 2024
10 checks passed
@ocefpaf ocefpaf deleted the remove_pkg_resources branch April 11, 2024 10:09
@ChrisBarker-NOAA
Copy link
Contributor

Thanks! Time for me to go update my PR, it may address the rest of these issues.

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.

4 participants