8000 replace pkg_resources with importlib in ska_helpers.get_version by javierggt · Pull Request #56 · sot/ska_helpers · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

replace pkg_resources with importlib in ska_helpers.get_version #56

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 31, 2024
Merged

replace pkg_resources with importlib in ska_helpers.get_version #56

merged 1 commit into from
Jan 31, 2024

Conversation

javierggt
Copy link
Collaborator
@javierggt javierggt commented Jan 18, 2024 •
edited by jeanconn
Loading

Description

This just replaces pkg_resources with importlib.

There are three points where I don't know yet whether changes are needed, but my best guess is what I implemented:

  • at the top, a warning is filtered. Is that still needed?

  • I removed a check:

    Check if the imported package __init__.py file has the same location
    as the distribution that was found. If working in a local git repo
    that does not have a .egg-info directory, get_distribution()
    will find an installed version

    I believe importlib does find the correct one (I checked on a cheta repo, see below)

  • The exceptionat the end used to catch DistributionNotFound and I made it catch metadata.PackageNotFoundError, but I do not know yet whether this will ever happen.

Fixes #55

Interface impacts

None

Testing

Unit tests

  • Mac (in 2023.9 with warning)
  • Mac (in 2024.0rc7 without warning)

Independent check of unit tests by Jean

  • Linux (ska3-flight -- I didn't focus on the warnings and left a pytest.ini in place -- just focused on tests continuing to pass).
jeanconn-fido> ska3
ska3-jeanconn-fido> pytest
====================================================== test session starts =======================================================
platform linux -- Python 3.10.8, pytest-7.2.1, pluggy-1.0.0
rootdir: /proj/sot/ska/jeanproj/git, configfile: pytest.ini
plugins: anyio-3.6.2, timeout-2.1.0
collected 63 items                                                                                                               

ska_helpers/retry/tests/test_retry.py ..........                                                                           [ 15%]
ska_helpers/tests/test_chandra_models.py ..................                                                                [ 44%]
ska_helpers/tests/test_git_helpers.py .                                                                                    [ 46%]
ska_helpers/tests/test_paths.py ......                                                                                     [ 55%]
ska_helpers/tests/test_run_info.py ..                                                                                      [ 58%]
ska_helpers/tests/test_utils.py ..........................                                                                 [100%]

====================================================== 63 passed in 13.65s =======================================================
ska3-jeanconn-fido> git rev-parse HEAD
47518a2dba31afcd68b1b84d87e891a1990f8c56

Functional tests

In [1]: import Ska.arc5gl
   ...: import ska_arc5gl
   ...: import Ska.engarchive
   ...: import cheta
   ...: import ska_helpers  # in local git repo
   ...: 

In [2]: print(f"""
   ...: Ska.arc5gl: {Ska.arc5gl.__version__}
   ...: ska_arc5gl: {ska_arc5gl.__version__}
   ...: Ska.engarchive: {Ska.engarchive.__version__}
   ...: cheta: {cheta.__version__}
   ...: ska_helpers: {ska_helpers.__version__}
   ...: """)

Ska.arc5gl: 4.0.1
ska_arc5gl: 4.0.1
Ska.engarchive: 4.60.0
cheta: 4.60.0
ska_helpers: 0.14.1.dev1+g47518a2

In the cheta repository, made sure to remove all egg directories, changed to the speedy branch, and then:

In [1]: import cheta

In [2]: cheta.__version__
Out[2]: '4.61.1.dev3+g55c4676'

jeanconn
jeanconn reviewed Jan 23, 2024
View reviewed changes
ska_helpers/version.py
@@ -22,7 +22,7 @@
warnings.filterwarnings(
"ignore", message=r"Module \w+ was already imported", category=UserWarning
)
from pkg_resources import DistributionNotFound, get_distribution
from importlib import resources, metadata
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems fine, though unlike pkg_resources, we're already importing importlib at the top of the file. Does this need to be in this catch_warnings block?

Copy link
Member

Choose a reason for hiding this comment

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

Good point. I'm fighting two instincts, (1) cut any code that doesn't seem necessary (and re-test) and (2) let it go and approve and move on. I'm honestly not sure, but otherwise this PR looks good and we should get it merged one way or the other.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These two sub-modules are not imported by the import statement at the top of the file, so they have to be explicitly imported. This goes to say that the fact that importlib was already imported says nothing about whether the warning would be issued here

I don't know whether importing without the warnings filter would trigger the warning. I do not remember if there were special cases when this was triggered, or if a simple call to testr is enough to check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just removed the filter warning in the master branch, and I did not get this warning, so I do not know in which circumstances this warning was issued to begin with.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems fine to leave this as is.

@jeanconn
Copy link
Contributor

I'm also wondering what could make sense for a versions unit test, but that's not a lien on this PR.

@javierggt javierggt merged commit d80b23e into master Jan 31, 2024
@javierggt javierggt mentioned this pull request Feb 13, 2024
Closed
This was referenced Mar 6, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeanconn jeanconn jeanconn approved these changes

@taldcroft taldcroft Awaiting requested review from taldcroft

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DeprecationWarning: pkg_resources is deprecated as an API
3 participants
@javierggt @jeanconn @taldcroft
0