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 try/finally block to yield in reporter.py #8508

Merged
merged 1 commit into from
Dec 6, 2019
Merged

Add try/finally block to yield in reporter.py #8508

merged 1 commit into from
Dec 6, 2019

Conversation

keisukefukuda
Copy link
Member

@keisukefukuda keisukefukuda commented Dec 5, 2019

background

In reporter.py, a logging context stack is maintained as its internal thread-local variable named _thread_local.reporters.

In Reporter.scope() function, a Reporter object is pushed and popped to the stack in __enter__() and __exit__() function.

the bug

If the yield omits an exception, the __exit__() is not called and the stack becomes an inconsistent state. In most use cases, such runtime error causes program termination thus the bug does not matter a lot.

However, in rare cases, such as #8384, it matters.
The bug forces the whole pytest process to terminate during a test.

fix

This PR fixes the problem by adding try/finally block around the yield sentence so the pop operation is done correctly and the stack is kept consistent.

Appendix:

The error message from #8384 test looks like this:

tests/chainermn_tests/links_tests/test_batch_normalization.py .F

=================================== FAILURES ===================================
____________ test_multi_node_bn_cpu[NaiveCommunicator-mpi-float16] _____________

communicator_class = <class 'chainermn.communicators.naive_communicator.NaiveCommunicator'>
backend = 'mpi', dtype = <class 'numpy.float16'>

    @pytest.mark.parametrize(('communicator_class', 'backend', 'dtype'), [
        (NaiveCommunicator, 'mpi', numpy.float16),
        (NaiveCommunicator, 'mpi', numpy.float32),
        (NaiveCommunicator, 'mpi', chainer.mixed16)])
    def test_multi_node_bn_cpu(communicator_class, backend, dtype):
        comm = create_communicator(communicator_class, mpi_comm,
                                   use_gpu=False)
>       check_multi_node_bn(comm, backend=backend, dtype=dtype)

tests/chainermn_tests/links_tests/test_batch_normalization.py:240:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
tests/chainermn_tests/links_tests/test_batch_normalization.py:137: in check_multi_node_bn
    l1 = m1(x, y)
chainer/link.py:287: in __call__
    out = forward(*args, **kwargs)
chainer/links/model/classifier.py:145: in forward
    reporter.report({'loss': self.loss}, self)
chainer/reporter.py:246: in report
    current.report(values, observer)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

@keisukefukuda keisukefukuda added the cat:bug Bug report or fix. label Dec 5, 2019
@emcastillo emcastillo self-assigned this Dec 5, 2019
@emcastillo
Copy link
Member

Jenkins, test this please

@emcastillo emcastillo added st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. to-be-backported Pull request that should be backported. labels Dec 6, 2019
@emcastillo emcastillo added this to the v7.1.0 milestone Dec 6, 2019
@chainer-ci
Copy link
Member

Jenkins CI test (for commit e5e6595, target branch master) succeeded!

@mergify mergify bot merged commit c66e960 into chainer:master Dec 6, 2019
@chainer-ci
Copy link
Member

@emcastillo This pull-request is marked as to-be-backported, but the corresponding backport PR could not be found. Could you check?

emcastillo pushed a commit to emcastillo/chainer that referenced this pull request Dec 9, 2019
…-reporter

Add try/finally block to yield in reporter.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:bug Bug report or fix. st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. to-be-backported Pull request that should be backported.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants