DM-40817: Allow skipping modules in ImportTestCase #173

arunkannawadi · 2023-09-18T19:12:28Z

Checklist

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

codecov · 2023-09-18T19:24:42Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01% 🎉

Comparison is base (42b57e1) 94.28% compared to head (413e2b0) 94.30%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #173      +/-   ##
==========================================
+ Coverage   94.28%   94.30%   +0.01%     
==========================================
  Files          46       46              
  Lines        3169     3180      +11     
==========================================
+ Hits         2988     2999      +11     
  Misses        181      181

Files Changed	Coverage Δ
python/lsst/utils/tests.py	`79.22% <100.00%> (+0.35%)`	⬆️
tests/test_import.py	`100.00% <100.00%> (ø)`

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

arunkannawadi · 2023-09-18T19:58:12Z

Do we actually support python 3.9? There's a strange error that doesn't occur with python 3.10 or 3.11 and I am not sure if I should dig deep into this.

timj · 2023-09-18T20:11:51Z

The error on 3.9 seems to be an interaction between importlib.resources and how it scans that package that lives in the tests directory. I think we have three options:

Change tests.py to use importlib_resources on python 3.9 (which is the 3.11 version of the code) and add dependency.
Give up on 3.9 completely
Skip the test case on python 3.9.

None of these options are more than a few lines to fix. My philosophy for utils is that, given it's a very low level package, I try to support as old a python as I can until I get annoyed by doing so.

timj · 2023-09-18T20:14:26Z

I think my preference is to see if importlib_resources works on 3.9.

import sys
if sys.version_info < (3, 10, 0):
    import importlib_resources as resources
else:
    from importlib import resources

or something and see if that fixes it. (will need importlib_resources added as dependency to pyproject.toml and requirements.txt and I don't think there is a way to only make that happen on 3.9 distros)

timj · 2023-09-18T20:20:54Z

tests/test_import.py

+
+    def test_import(self):
+        """Test that we can import utils."""
+        import import_test  # noqa: F401


This import might be a no-op depending on what other tests have already run. (pkg_resources used to import parent classes as it went but I'm not sure if importlib.resources does so)

timj · 2023-09-18T20:21:38Z

python/lsst/utils/tests.py

+    SKIP_MODULES: ClassVar[Mapping[str, Iterable[str]]] = {}
+    """Modules to be skipped importing as key-value pairs.
+
+    The key is the package name and the value is a set of module names to skip.


Is "module name" the right word here? Aren't these file names? ie you are requiring the .py extension.

I could change it to SKIP_FILES. While on that, I should also change the type of the value to be Container[str], since I only need in to work.

python/lsst/utils/tests.py

timj · 2023-09-18T20:26:46Z

python/lsst/utils/tests.py

@@ -43,7 +43,11 @@
 import unittest
 import warnings
 from collections.abc import Callable, Iterable, Iterator, Mapping, Sequence
-from importlib import resources
+import sys


timj · 2023-09-18T20:27:34Z

python/lsst/utils/tests.py

-from importlib import resources
+import sys
+if sys.version_info < (3, 10, 0):
+    import importlib_resources as resources


I think that importlib_resources has to be added as a dependency to pyproject.toml and requirements.txt

from importlib import resources seems to work in python3.9 anyway. Does that have a different behavior despite the same name?

It's not the same name. It's different. importlib_resources is a PyPI package that allows older pythons to use the newer implementation of importlib.resources. That's why I was saying that you need to add importlib_resources as a dependency in pyproject.toml and requirements.txt.

importlib.resources was working fine on python 3.9 until you caused it trouble by asking it to scan that package inside the tests directory. The reason I'm asking you to try importlib_resources on python 3.9 is that they might have fixed a bug in that area which explains why it worked fine on 3.10.

Okay, this patch works! Thanks Tim, will clean up the changes, address the other minor comments and merge.

timj

Some minor comments.

timj · 2023-09-18T21:18:33Z

python/lsst/utils/tests.py

@@ -346,6 +351,12 @@ class ImportTestCase(unittest.TestCase):
    PACKAGES: ClassVar[Iterable[str]] = ()
    """Packages to be imported."""

+    SKIP_FILES: ClassVar[Mapping[str, Container[str]]] = {}


Don't forget to change the doc string.

arunkannawadi force-pushed the tickets/DM-40817 branch from 4151d6e to 12d4430 Compare September 18, 2023 19:22

arunkannawadi changed the title ~~Allow skipping modules in ImportTestCase~~ DM-40817: Allow skipping modules in ImportTestCase Sep 18, 2023

arunkannawadi force-pushed the tickets/DM-40817 branch from 150e3e3 to a8c4c99 Compare September 18, 2023 19:55

arunkannawadi force-pushed the tickets/DM-40817 branch from a8c4c99 to 9ec39ba Compare September 18, 2023 19:59

timj reviewed Sep 18, 2023

View reviewed changes

python/lsst/utils/tests.py Outdated Show resolved Hide resolved

timj reviewed Sep 18, 2023

View reviewed changes

timj approved these changes Sep 18, 2023

View reviewed changes

arunkannawadi force-pushed the tickets/DM-40817 branch 2 times, most recently from 607d4d1 to c643c4b Compare September 18, 2023 21:05

timj reviewed Sep 18, 2023

View reviewed changes

arunkannawadi added 3 commits September 19, 2023 08:35

Import importlib_resources conditionally

c7e0c1c

Allow skipping files in ImportTestCase

4bc05f5

Add a file to doc/changes

413e2b0

arunkannawadi force-pushed the tickets/DM-40817 branch from c643c4b to 413e2b0 Compare September 19, 2023 15:37

arunkannawadi merged commit 3bf5ff2 into main Sep 19, 2023
15 checks passed

arunkannawadi deleted the tickets/DM-40817 branch September 19, 2023 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-40817: Allow skipping modules in ImportTestCase #173

DM-40817: Allow skipping modules in ImportTestCase #173

arunkannawadi commented Sep 18, 2023 •

edited

codecov bot commented Sep 18, 2023 •

edited

arunkannawadi commented Sep 18, 2023

timj commented Sep 18, 2023

timj commented Sep 18, 2023

timj Sep 18, 2023 •

edited

timj Sep 18, 2023

arunkannawadi Sep 18, 2023

timj Sep 18, 2023

timj Sep 18, 2023

arunkannawadi Sep 18, 2023

timj Sep 18, 2023

timj Sep 18, 2023

arunkannawadi Sep 18, 2023

timj left a comment

timj Sep 18, 2023

DM-40817: Allow skipping modules in ImportTestCase #173

DM-40817: Allow skipping modules in ImportTestCase #173

Conversation

arunkannawadi commented Sep 18, 2023 • edited

Checklist

codecov bot commented Sep 18, 2023 • edited

Codecov Report

arunkannawadi commented Sep 18, 2023

timj commented Sep 18, 2023

timj commented Sep 18, 2023

timj Sep 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arunkannawadi commented Sep 18, 2023 •

edited

codecov bot commented Sep 18, 2023 •

edited

timj Sep 18, 2023 •

edited