search

NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
17.39k stars 13.61k forks source link

pytestCheckHook: Switch from `python -m pytest` to simply `pytest`? #255262

Open doronbehar opened 1 year ago

doronbehar commented 1 year ago

Summary of issue

Many packages in Nixpkgs that use cython extension modules, demonstrate some issues with running tests via pytestCheckHook, credit to @natsukium for helping figuring that out.

Generally, the problem is that the extension modules are installed at the installCheckPhase at $out, and are not $PWD. This for instance may cause differences between running pytest and running python -m pytest - hence the issue title. What's rather peculiar, is the fact that all packages that manage to overcome that use different schemes. The most common of them all is:

preCheck = ''
  cd $out
'';

Below is a list of such packages, and PRs that improved the situation a bit:

Below is the original issue description, opened before further details were investigated. Hence the issues there may be a bit outdated.

Original issue description

Describe the bug

While trying to enable tests for pyerfa, I noticed that I got circular imports errors as explained upstream here. I then ran git grep circular import and learned that another Python package, pyrevolve is experiencing the same issue on NixOS:

https://github.com/NixOS/nixpkgs/blob/46688f8eb5cd6f1298d873d4d2b9cf245e09e88e/pkgs/development/python-modules/pyrevolve/default.nix#L36-L46

Steps To Reproduce

For each package that comes out in a git grep circular import search, I'll list what I learned, from most interesting, to less interesting:

python3.pkgs.pyrevolve

Try to build it with this patch:

diff --git i/pkgs/development/python-modules/pyrevolve/default.nix w/pkgs/development/python-modules/pyrevolve/default.nix
index 754baf91ad38..33df5d177f4d 100644
--- i/pkgs/development/python-modules/pyrevolve/default.nix
+++ w/pkgs/development/python-modules/pyrevolve/default.nix
@@ -38,7 +38,7 @@ buildPythonPackage rec {
   # ImportError: cannot import name 'crevolve' from partially initialized module 'pyrevolve'
   # (most likely due to a circular import)
   checkPhase = ''
-    pytest
+    python -m pytest
   '';

   pythonImportsCheck = [

Which does the same as replacing pytest with pytestCheckHook, and removing the overriding checkPhase. Note how the tests now fail.

python3.pkgs.orange3

Seems like a very similar case to what happens in python3.pkgs.pyrevolve, but replacing python -m pytest with pytest doesn't resolve the issue. I also haven't opened an issue upstream because I'm not sure whether this is our fault or theirs.

I tested this issue using this patch ```diff diff --git i/pkgs/development/python-modules/orange3/default.nix w/pkgs/development/python-modules/orange3/default.nix index cff4a603c846..4a9051548655 100644 --- i/pkgs/development/python-modules/orange3/default.nix +++ w/pkgs/development/python-modules/orange3/default.nix @@ -24,6 +24,7 @@ , pyqtgraph , pyqtwebengine , python +, pytest , python-louvain , pythonOlder , pyyaml @@ -104,8 +105,13 @@ let baycomp ]; - # FIXME: ImportError: cannot import name '_variable' from partially initialized module 'Orange.data' (most likely due to a circular import) (/build/source/Orange/data/__init__.py) - doCheck = false; + doCheck = true; + nativeCheckInputs = [ + pytest + ]; + checkPhase = '' + pytest + ''; pythonImportsCheck = [ "Orange" "Orange.data._variable" ]; ```

python3.pkgs.primer3

The circular import issue is mentioned there in a comment, but that comment seems maybe inaccurate to me, and the same testing failure is observed when adding pytest to nativeCheckInputs, and when running manually pytest in checkPhase. Comment improvement is in https://github.com/NixOS/nixpkgs/pull/255260

python3.pkgs.xdot

Don't know why was the purpose of the circular import comment, maybe it was outdated. Now a fix for tests and update is available in https://github.com/NixOS/nixpkgs/pull/255254 .

python3.pkgs.iniconfig

Not exactly this issue, but rather it is a form of https://github.com/NixOS/nixpkgs/issues/63168 . Improvement for the comment near it's doCheck = false; is in https://github.com/NixOS/nixpkgs/pull/255256 .

Expected behavior

No difference between pytest and python -m pytest.

Additional context

Problem was investigated also in https://discourse.nixos.org/t/python-package-testing-circular-import-in-a-version-py/33019/ .

Notify maintainers

@FRidh . Maybe python3.pkgs.orange3, and python3.pkgs.pyrevolve maintainers @lucasew and @AtilaSaraiva will be also interested.

nixos-discourse commented 1 year ago

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/python-package-testing-circular-import-in-a-version-py/33019/5

natsukium commented 1 year ago

It is not a circular import, but this is a problem often encountered in extension modules generated by cython and others.

some examples - https://github.com/NixOS/nixpkgs/pull/240183#pullrequestreview-1507120301 : use pytest directly with a flag `--import-mode append` - https://github.com/NixOS/nixpkgs/pull/238058#discussion_r1243859098 : change the current directory - https://github.com/NixOS/nixpkgs/pull/102729#discussion_r517844524 : remove source directory

python -m pytest adds the current directory to the head of the search path, so python imports packages in $src/ instead of packages installed in $out/{python.sitePackages}/. https://docs.pytest.org/en/7.1.x/explanation/pythonpath.html#invoking-pytest-versus-python-m-pytest I don't think this is the expected behavior of pytestCheckHook.

lucasew commented 1 year ago

Just for reference:

pytestCheckHook is defined in pkgs/development/interpreters/python/hooks/pytest-check-hook.sh

And I think that -m behavior is to allow using a interpreter specific for testing. I don't know the usecases of this approach exactly as I am not the author of the hook.

Do you think that an option to disable this behaviour and use pytest directly would be better?

Like instead of that eval "@pythonCheckInterpreter@ $args" some kind of check that if a flag is enabled then pytest $args?

doronbehar commented 1 year ago

Like instead of that eval "@pythonCheckInterpreter@ $args" some kind of check that if a flag is enabled then pytest $args?

Definitely that would be nice, but I'm not sure that's the only thing that is required here. At https://github.com/astropy/astropy/issues/15316#issuecomment-1722190547 I summarized some my findings regarding pytest vs python -m pytest.

It is not a circular import, but this is a problem often encountered in extension modules generated by cython and others.

some examples

Thanks for the examples! Indeed for the packages I tried to update and enable tests (pyerfa and astropy) was using cd $out in preCheck. See also https://github.com/NixOS/nixpkgs/pull/255471 .

On the other hand over at https://github.com/NixOS/nixpkgs/pull/255260 a different solution was implemented eventually.