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

Add more tests of package traversal #906

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Sep 20, 2024

This PR adds tests (including some xfailed ones) demonstrated patterns of package/subpackage access via import and importlib that are expected to work correctly for both normal and editable installs. Related to #807.

@vyasr
Copy link
Contributor Author

vyasr commented Sep 20, 2024

@LecrisUT this PR should capture some of the cases you mentioned in #808 (comment). Are there others that you would like to see added?

@LecrisUT
Copy link
Collaborator

Looks promising :). I'll need to draw it out on a paper to follow it 😅 , I'll come back to you after that. At first glance, I think the only part not covered is having both relative and absolute paths in the __init__.py, but maybe that is sufficiently covered by the other test.

@vyasr
Copy link
Contributor Author

vyasr commented Sep 23, 2024

@henryiii this is currently a draft mostly so that I can collect ideas from you and @LecrisUT on other tests we might want to add, so let me know what you think and I can update this PR accordingly before opening it up for review.

Copy link
Collaborator

@LecrisUT LecrisUT left a comment

Choose a reason for hiding this comment

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

Regarding the coverage, right now it seems the import only cover python packages, the c methods must also be imported there.

Most of the imports only cover 1 level deep imports, and are a bit redundant. I would say to minimize it and comment what is being covered:

# pkg/__init__.py

# Covering importing one-level deep from 1st level pakage
from .py_mod import py_square1
from .c_mod import c_square1

# Coverging importing 2-levels deep from 1st level package
from .subA.py_mod import py_square2
from .subA.c_mod import c_square2
# pkg/subA/__init__.py

# Covering importing one-level deep from 2nd level pakage
from .py_mod import py_square3
from .c_mod import c_square3

# Coverging importing relative paths backwards
from ..subB.py_mod import py_square4
from ..subB.c_mod import c_square4

I suck at naming conventions, hope you can figure a good one.

@LecrisUT
Copy link
Collaborator

On the overall structure of the files, I think it's a good skeleton to cover all the cases, we just need to cover the navigations within there. Just a few notes:

  • there are 2 files missing: a top-level py_mod.py and a top-level c_mod.c i.e. not belonging to pkg.*
  • namespace packages are not covered:
    • src namespace packages, i.e. src1/pkg/py_modA.py + src2/pkg/my_modB.py that would be installed in site_packages/pkg/py_mod{A,B}.py and resolved as pkg.py_modA/pkg.py_modB
    • installed namespace packages, i.e. site_packages/other_pkg/py_mod{A,B}.py where there is no site_packages/other_pkg/__init__.py and those resolve as other_pkg.py_modA.py/other_pkg.py_modB.py. An example of this is jaraco.* packages.

@vyasr
Copy link
Contributor Author

vyasr commented Oct 18, 2024

OK I think I've addressed most of the requests. Some notes:

  • I have not yet included any imports traversing up the hierarchy because those would cause circular imports. I'm not sure how you'd like that to be handled here.
  • I added the top-level pure and extension modules but I don't know what you'd like me to do with them.
  • I haven't added namespace packages yet. I don't understand the importance of the src vs installed cases that you want to address.

@vyasr vyasr requested a review from LecrisUT October 18, 2024 23:25
Copy link
Collaborator

@LecrisUT LecrisUT left a comment

Choose a reason for hiding this comment

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

* I have not yet included any imports traversing up the hierarchy because those would cause circular imports. I'm not sure how you'd like that to be handled here.

You should be able to break the circularity, just limit the import in the __init__.py files. Specifically, I think you can eliminate the imports in sub_a and sub_d and use sub_b and sub_c to test relative imports upwards. Maybe some other names would be helpful to visualize which is the main tree that is tested (i.e. pkg, pkg.sub_b, pkg.sub_b.sub_c) and the auxiliary branches that are used to cover the navigation cases (e.g. ..sub_a, ...sub_a being called from the main tree)

* I added the top-level pure and extension modules but I don't know what you'd like me to do with them.

Just have them in a import statement at pkg/__init__.py should be fine

* I haven't added namespace packages yet. I don't understand the importance of the src vs installed cases that you want to address.

The difference is how editable installs handle it. When they are installed in the site_package, we can mostly ignore all package traversal since it would be handled by python std itself. If there is an error it's an error of the installation. When they are in src and they are handled by editable install, then we need to make sure that our script is configured appropriately and all navigation cases are covered. The testing of both installed and editable would just make it easier to figure out if it is indeed a editable install issue or not, and it's good to have parity confirmation in general.


Note that we should also cover the importlib.resources navigation with testfile as the target (one located in the CMake tree that you have and one located in the src tree)

You can write it as asserts in the __init__.py, e.g.

from importlib.resources import files

cmake_file = files("..") / "testfile"
assert cmake_file.read_text() == "This is the file"

Feel free to one-line it if needed.


This is a tall order, but do you know how to use draw.io? We could make a simple drawing of the tree and put some notes there, and have it under vcs here.

@LecrisUT LecrisUT force-pushed the test/editable_package_traversal branch from e119367 to e39cf5a Compare March 19, 2025 18:00
@LecrisUT
Copy link
Collaborator

LecrisUT commented Mar 19, 2025

@vyasr sorry for taking so long to get to this. I have updated the PR and added a few more navigation cases that I was referring to last time. I have also simplified the test_importlib_resources, do you want to add a few more checks in there?

Thanks again for working on this, you've done a great job on this, I only had some nitpicks that I wanted to cover, which I thought would be clearer to show with the commits themselves, hope it's ok with you.

One final comment, would you want to try and change _setup_package_for_editable_layout_tests to be a @pytest.fixture. I think it would be a good exercise to figure out how to make the fixture boilerplates more compact. See test_editable_unit.editable_package.

@LecrisUT LecrisUT force-pushed the test/editable_package_traversal branch from 2918fad to 3beb8d5 Compare March 19, 2025 18:48
@LecrisUT LecrisUT force-pushed the test/editable_package_traversal branch from 3beb8d5 to cc63b29 Compare March 19, 2025 19:09
@LecrisUT LecrisUT requested a review from henryiii March 19, 2025 19:19
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.

2 participants