No tape-able operations at module level in adjoint tests#4867
No tape-able operations at module level in adjoint tests#4867JHopeCollins wants to merge 2 commits intomainfrom
Conversation
|
|
||
|
|
||
| @pytest.fixture(scope="module") | ||
| def set_module_annotation(): |
There was a problem hiding this comment.
Is having module scope taping ever a good idea?
There was a problem hiding this comment.
I do not think so. The previous version of this fixture that was duplicated in each file actually called continue_annotation at the beginning of each module. I've changed that to pause_annotation here and enabled annotation on a per-test basis in set_test_tape (pending agreement on the pyadjoint PR).
There was a problem hiding this comment.
Ah. I got confused by the name of this fixture. Now this seems to be fairly close to the check_empty_tape autouse fixture just above this. I wonder if these could be combined somehow.
I would have thought maybe we'd like to have 2 fixtures, an autouse one like
@pytest.fixture(autouse=True) # for every test
def assert_untaped():
assert not annotate_tape()
yield
assert not annotate_tape()then one we'd explicitly use for when we want taping
@pytest.fixture
def enable_taping():
enable_annotation()
yield
pause_annotation()There was a problem hiding this comment.
And creating a new tape each time is also a good idea.
There was a problem hiding this comment.
I don't know if we can guarantee the order that fixtures of the same scope will be called. I think reducing it to two fixtures is doable, but with:
- a module-level fixture called for all modules that asserts that annotation is off at entry and exit
- a function-level fixture that is auto-used in the adjoint test files (like I've got already) that enables the taping and sets a new per-test tape.
What do you think?
There was a problem hiding this comment.
The pytest docs suggest that we can control the order of fixtures.
There was a problem hiding this comment.
I'm keen to see if we can localise all the tape handling to being per-test.
There was a problem hiding this comment.
If the CI passes I'll have a go at simplifying the taping fixtures
I was getting some very odd and difficult to reproduce errors with the adjoint tests, for example a test was passing if I only passed that file to pytest, but was failing if I passed the test directory and just selected that test using
-k.It seems that if there are tape-able operations at module level in the adjoint tests, they pollute the global state when that test file is collected, even if the tests in that module are not run. This still appeared to happen even when I tried to switch off annotations with the module-level fixture, and used
set_working_tapeto wrap all my taped operations!This PR removes any tape-able operations from module level and stuffs them either inside the tests or inside fixtures which are called on a test-by-test basis when the tests are run (not when they are collected).
I also put the fixtures that manage the taping in the adjoint tests into the
conftest.py. We still need to "call" them in each adjoint test file by writing file-level fixtures that call the conftest fixtures, but at least all the logic is in one place.Uses dolfin-adjoint/pyadjoint#245 to slightly simplify the use of
set_working_tapein the fixtures.