Missing item in dict from parametrize

[cdeil]

A week or so ago, a very strange error appeared in our CI build here which looks like a Pytest bug to me:
https://dev.azure.com/gammapy/gammapy/_build/results?buildId=25&view=logs

platform linux2 -- Python 2.7.15, pytest-3.10.1, py-1.7.0, pluggy-0.8.0

Our test code is

@pytest.mark.parametrize("case", get_test_cases())
def test_counts_predictor(case):
    desired = case.pop("npred")

and as you can see in https://github.com/gammapy/gammapy/blob/fb7b218f1e6a774271b80668a4e8dd31753327dd/gammapy/spectrum/tests/test_utils.py#L139-L141 the get_test_cases function returns a list of dicts, and those dicts do have an "npred" key.

Yet in this Python 2 build, pytest passes a dict where the "npred" key is missing:

2018-11-13T11:12:21.4346365Z _________________________ test_counts_predictor[case0] _________________________
2018-11-13T11:12:21.4346436Z 
2018-11-13T11:12:21.4347229Z case = {'e_true': <Quantity [  0.1       ,   0.10597662,   0.11231045,   0.11902282,
2018-11-13T11:12:21.4347393Z            ...927389,
2018-11-13T11:12:21.4347808Z             84.017...,  89.03890941,  94.36043101, 100.        ] TeV>, 'model': <gammapy.spectrum.models.PowerLaw object at 0x7f44e5e31990>}
2018-11-13T11:12:21.4347950Z 
2018-11-13T11:12:21.4348083Z     @pytest.mark.parametrize("case", get_test_cases())
2018-11-13T11:12:21.4348380Z     def test_counts_predictor(case):
2018-11-13T11:12:21.4348505Z >       desired = case.pop("npred")
2018-11-13T11:12:21.4348837Z E       KeyError: u'npred'
2018-11-13T11:12:21.4348915Z 
2018-11-13T11:12:21.4349045Z gammapy/spectrum/tests/test_utils.py:141: KeyError
2018-11-13T11:12:21.4366992Z _________________________ test_counts_predictor[case1] _________________________
2018-11-13T11:12:21.4367112Z 
2018-11-13T11:12:21.4367811Z case = {'aeff': <gammapy.irf.effective_area.EffectiveAreaTable object at 0x7f44e5e316d0>, 'livetime': <Quantity 10. h>, 'model': <gammapy.spectrum.models.PowerLaw object at 0x7f44e5e318d0>}
2018-11-13T11:12:21.4367983Z 
2018-11-13T11:12:21.4368082Z     @pytest.mark.parametrize("case", get_test_cases())
2018-11-13T11:12:21.4368201Z     def test_counts_predictor(case):
2018-11-13T11:12:21.4368324Z >       desired = case.pop("npred")
2018-11-13T11:12:21.4368609Z E       KeyError: u'npred'
2018-11-13T11:12:21.4368710Z 
2018-11-13T11:12:21.4368802Z gammapy/spectrum/tests/test_utils.py:141: KeyError

This code hasn't changed on our side, and I cannot reproduce the error locally, on MacOS with Python 2.7 and pytest 3.10.1 as used in CI, and the issue doesn't appear in our many other CI builds, only in this one build on Azure. So it looks like an edge case that is hard to reproduce.

Could you please have a look and see if this is a pytest bug?
I think this way of using parametrize should be OK, to have a function that makes a list of dicts, and then to pop an item from that dict in the test, no?

cc @adonath from Gammapy.

[Zac-HD]

I think this way of using parametrize should be OK, to have a function that makes a list of dicts, and then to pop an item from that dict in the test, no?

You'd be better off having the function return a list of tuples, and using the multiple-argument form @parametrize('model,e_true,npred', ...).

I honestly don't know the root cause of this issue, but mutating state like this when you can just pass the parts separately seems like asking for trouble. Worst case, you could sidestep the issue by adding case = dict(case) as the first line of the test!

[RonnyPfannschmidt]

try if it happens with --assert=plain - might be a deeper bug

also try dropping unicode literals

[asottile]

weird, I can reproduce this -- it goes away if I apply this diff:

diff --git a/gammapy/_astropy_init.py b/gammapy/_astropy_init.py
index 8f5041f2..d9cedc39 100644
--- a/gammapy/_astropy_init.py
+++ b/gammapy/_astropy_init.py
@@ -106,6 +106,7 @@ def test(package=None, test_path=None, args=None, plugins=None,
         explicitly updating the package template.
 
     """
+    return
     test_runner = _get_test_runner()
     return test_runner.run_tests(
         package=package, test_path=test_path, args=args,

(I was trying to make the tests/__init__.py "test" faster so I could get a better debug loop, but this causes the test to magically pass)

I suspect something fishy is happening with the astropy test-runner helper? (I also suspect you're running all of your tests twice!)

[asottile]

Here's a minimal reproduction:

import pytest


def test_self():
    pytest.main([__file__, '-k', 'not test_self'])


def make_dicts():
    return [{'hello': 'world'}, {'hello': 'world2'}]


@pytest.mark.parametrize('case', make_dicts())
def test_case(case):
    case.pop('hello')
    assert not case

$ pytest t.py
============================= test session starts ==============================
platform linux2 -- Python 2.7.15rc1, pytest-3.10.1, py-1.7.0, pluggy-0.8.0
rootdir: /tmp/t2, inifile:
collected 3 items                                                              

t.py .FF                                                                 [100%]

=================================== FAILURES ===================================
_______________________________ test_case[case0] _______________________________

case = {}

    @pytest.mark.parametrize('case', make_dicts())
    def test_case(case):
>       case.pop('hello')
E       KeyError: 'hello'

t.py:14: KeyError
_______________________________ test_case[case1] _______________________________

case = {}

    @pytest.mark.parametrize('case', make_dicts())
    def test_case(case):
>       case.pop('hello')
E       KeyError: 'hello'

t.py:14: KeyError
====================== 2 failed, 1 passed in 0.08 seconds ======================

[cdeil]

@asottile - Thank you for debugging this!

Do you see the issue with pytest only? Or does it only appear in an environment where Astropy or Gammapy is installed?

[asottile]

my minimal case doesn't involve astropy or gammapy, that said -- it's doing something (imo) inane (running pytest inside of pytest)

[asottile]

applying a patch like this makes this a more-apparent user error:

         self.excinfo = excinfo
 
 
+def non_reentrant(fn):
+    @functools.wraps(fn)
+    def non_reentrant_wrapper(*args, **kwargs):
+        if hasattr(fn, non_reentrant.__name__):
+            raise RuntimeError('{}.{} is not reentrant'.format(fn.__module__, fn.__name__))
+
+        setattr(fn, non_reentrant.__name__, None)
+        try:
+            return fn(*args, **kwargs)
+        finally:
+            delattr(fn, non_reentrant.__name__)
+    return non_reentrant_wrapper
+
+
+@non_reentrant
 def main(args=None, plugins=None):
     """ return exit code, after performing an in-process test run.
 
     :arg args: list of command line arguments.

$ pytest t.py
============================= test session starts ==============================
platform linux2 -- Python 2.7.15rc1, pytest-4.0.0, py-1.7.0, pluggy-0.8.0
rootdir: /tmp/t2, inifile:
collected 3 items                                                              

t.py F..                                                                 [100%]

=================================== FAILURES ===================================
__________________________________ test_self ___________________________________

    def test_self():
>       pytest.main([__file__, '-k', 'not test_self'])

t.py:5: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

args = (['/tmp/t2/t.py', '-k', 'not test_self'],), kwargs = {}

    @functools.wraps(fn)
    def non_reentrant_wrapper(*args, **kwargs):
        if hasattr(fn, non_reentrant.__name__):
>           raise RuntimeError('{}.{} is not reentrant'.format(fn.__module__, fn.__name__))
E           RuntimeError: _pytest.config.main is not reentrant

venv/local/lib/python2.7/site-packages/_pytest/config/__init__.py:51: RuntimeError
====================== 1 failed, 2 passed in 0.03 seconds ======================

[asottile]

also confirming, this isn't a python2.x only issue:

$ pytest t.py 
============================= test session starts ==============================
platform linux -- Python 3.6.6, pytest-4.0.0, py-1.7.0, pluggy-0.8.0
rootdir: /tmp/t2, inifile:
collected 3 items                                                              

t.py .FF                                                                 [100%]

=================================== FAILURES ===================================
_______________________________ test_case[case0] _______________________________

case = {}

    @pytest.mark.parametrize('case', make_dicts())
    def test_case(case):
>       case.pop('hello')
E       KeyError: 'hello'

t.py:14: KeyError
_______________________________ test_case[case1] _______________________________

case = {}

    @pytest.mark.parametrize('case', make_dicts())
    def test_case(case):
>       case.pop('hello')
E       KeyError: 'hello'

t.py:14: KeyError
====================== 2 failed, 1 passed in 0.07 seconds ======================

[RonnyPfannschmidt]

im inclined to consider this one a user-error

[asottile]

@RonnyPfannschmidt same -- should we make it a more apparent error with a patch similar (or identical) to the one I wrote above?

[cdeil]

my minimal case doesn't involve astropy or gammapy, that said -- it's doing something (imo) inane (running pytest inside of pytest)

@astrofrog @bsipocz @drdavella as Astropy test maintainers - thoughts?

The comment by @asottile seems to imply that Astropy does something non-recommended. Or are we just running it incorrectly?

[asottile]

to be clear: astropy's test helper doesn't seem problematic, but invoking it while already running pytest is

[RonnyPfannschmidt]

@asottile we should totally fail on unexpected recursion

but we should support intended recursion

[cdeil]

to be clear: astropy's test helper doesn't seem problematic, but invoking it while already running pytest is

So this is the problem?

    test_runner = _get_test_runner()
    return test_runner.run_tests(

https://github.com/gammapy/gammapy/blob/b06535acfffb643a7d3cc958b102e2980ac9986e/gammapy/_astropy_init.py#L109-L110

That's a file we got from the https://github.com/astropy/package-template .

@bsipocz @astrofrog - I don't find that code there now, maybe it has been recognised and fixed there in the meantime? If yes - is there a way to update Gammapy to get these fixes?

I will also note that we are seeing weird behaviour (no tests found or hanging after test collection) in some Gammapy CI builds, that I think started to appear after astropy/ci-helpers@7caa8fc when a newer Pytest version got used in CI, cc @pllim :

https://travis-ci.org/gammapy/gammapy/jobs/457879848#L2498
https://travis-ci.org/gammapy/gammapy/jobs/457879849#L2471

[cdeil]

In https://github.com/Cadair/package-template/commits/98e67d59d8246f915495a5684762c7bd6c78ee12/%7B%7B%20cookiecutter.package_name%20%7D%7D/%7B%7B%20cookiecutter.module_name%20%7D%7D/_%7B%7B%20cookiecutter._parent_project%20%7D%7D_init.py I see that @Cadair made changes to that file in the AStropy package-template, which probably resolve the issue we still have in Gammapy.

So is there a way to get such updates via a pull request? There used to be a bot sending such updates which I think @astrofrog you set up. Or do we have to try and figure out when / which updates to apply manually now for Astropy affiliated packages?

[astrofrog]

@cdeil - the automated updates were only ever for astropy-helpers, not the package-template changes, so you will need to copy over the required changes. We can discuss how to do this by email or on Slack if you run into any issues.

[cdeil]

So on the Astropy / Gammapy side, this issue was figured out: we are accidentally running pytest in pytest, and a solution was given by @nicoddemus in #4434 (comment) .

@asottile - Above you are considering pytest code changes. Would you still recommend some change, or should this issue simply be closed as mis-understanding / mis-usage?

[asottile]

my patch might be a bit too kneejerk -- I notice it breaks pytester and a few other things -- I think it's best to just chalk this up to misunderstanding and move on :)

[cdeil]

@asottile - OK, closing this issue. Thank you for helping us!