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

DM-48430: Add ability to get all installed packages from system #198

Merged
merged 12 commits into from
Jan 16, 2025

Conversation

timj
Copy link
Member

@timj timj commented Jan 15, 2025

Checklist

  • ran Jenkins
  • added a release note for user-visible changes to doc/changes

@timj
Copy link
Member Author

timj commented Jan 15, 2025

@kfindeisen can you give this a try please? One thing that might be an issue is that the "include_all" option historically includes all the EUPS build numbers. I'm not sure what my motivation for adding that was in #117 but it's an easy fix to remove the tags if we decide they get in the way.

@timj
Copy link
Member Author

timj commented Jan 15, 2025

Reading the Jira ticket I think @mfisherlevine wanted the weekly tag to show up in the version history. The problem is that it also reports build tags:

testdata_cfht gfc90949391 (b7235 b7237 b7238 b7239 b7240 b7241 b7242 b7243 b7244 b7245 b7246 b7247 b7248 b7249 b7250 b7251 b7252 b7253 b7254 b7255 b7256 b7257 b7258 b7259 b7260 b7261 b7262 b7263 b7264 b7265 b7266 b7267 b7268 b7269 b7270 b7272 b7273 b7274 b7275 b7277 b7278 b7279 b7282 b7283 b7284 b7285 b7286 b7287

Copy link

codecov bot commented Jan 15, 2025

Codecov Report

Attention: Patch coverage is 97.43590% with 1 line in your changes missing coverage. Please review.

Project coverage is 94.19%. Comparing base (bd976da) to head (f0fe60a).
Report is 13 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
python/lsst/utils/packages.py 96.15% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #198      +/-   ##
==========================================
+ Coverage   94.15%   94.19%   +0.03%     
==========================================
  Files          47       47              
  Lines        3440     3463      +23     
==========================================
+ Hits         3239     3262      +23     
  Misses        201      201              

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

@timj timj force-pushed the tickets/DM-48430 branch 2 times, most recently from 300f5d9 to bd6c905 Compare January 15, 2025 00:53
@kfindeisen
Copy link
Member

kfindeisen commented Jan 15, 2025

Well, it is faster (0.5 s vs. 2.1 s on S3DF). I'm not sure how to test its robustness, though -- I can't run PP without a container, and the lru_cache would suppress any changes anyway.

I didn't see any build numbers in testing.

@mfisherlevine
Copy link
Contributor

As ugly as the build tags are there, I don't think I have anything that relies on the weekly being there alone, and if I need it, I can always try adding a regex of some sort, so I think that's fine (at least for me).

Copy link
Member

@kfindeisen kfindeisen left a comment

Choose a reason for hiding this comment

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

I'm a bit concerned about the new unit test for fromSystem, since it requires the exact opposite of what the documentation says. Which is the intended behavior?

Also, please document the new features and bugfixes in doc/changes.

python/lsst/utils/packages.py Outdated Show resolved Hide resolved
python/lsst/utils/packages.py Outdated Show resolved Hide resolved
python/lsst/utils/packages.py Show resolved Hide resolved
python/lsst/utils/packages.py Show resolved Hide resolved
python/lsst/utils/packages.py Show resolved Hide resolved
tests/test_packages.py Show resolved Hide resolved
timj added 3 commits January 15, 2025 13:26
There has long been 2 variants for this function so a cache
size of two makes more sense.
This will be used by a future commit.
@timj timj force-pushed the tickets/DM-48430 branch from bd6c905 to 1a9053d Compare January 15, 2025 20:40
@timj timj requested a review from kfindeisen January 15, 2025 23:31
Copy link
Member

@kfindeisen kfindeisen left a comment

Choose a reason for hiding this comment

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

Looks much better, thanks! I won't speculate on why there are now 14 commits instead of a handful.

tests/test_packages.py Show resolved Hide resolved
doc/changes/DM-48430.feature.rst Outdated Show resolved Hide resolved
python/lsst/utils/packages.py Show resolved Hide resolved
python/lsst/utils/packages.py Outdated Show resolved Hide resolved
@timj
Copy link
Member Author

timj commented Jan 15, 2025

Thanks. I'll squash a bit now that we got agreement on the approach.

@timj timj force-pushed the tickets/DM-48430 branch from 75fe91a to b1fd8eb Compare January 15, 2025 23:55
timj added 8 commits January 15, 2025 16:55
Solely checks that True and False raise no errors.
This reports the versions of all installed python distributions.
getPythonPackages only reported python distributions where some
code has been imported.
This lets the test check for difference and missing without
having to know that a new import can be made.
@timj timj force-pushed the tickets/DM-48430 branch from b1fd8eb to f0fe60a Compare January 15, 2025 23:56
@timj timj merged commit 5a2f84e into main Jan 16, 2025
16 checks passed
@timj timj deleted the tickets/DM-48430 branch January 16, 2025 04:25
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