Ignite: Fix warnings in test

[vfdev-5]

🚀 Feature

Currently, we have several warnings raised during the tests from our side but not catched with pytest.warns:

• https://travis-ci.org/github/pytorch/ignite/jobs/689725736#L3577
• https://travis-ci.org/github/pytorch/ignite/jobs/689725736#L3589
• https://travis-ci.org/github/pytorch/ignite/jobs/689725736#L3593
• etc

We need to improve the code of tests by introducing

with pytest.warns(...):
    # code that raises a warning

[InCogNiTo124]

Hello, I'd like to help if I may. How much trouble do you think it would be?

[vfdev-5]

@InCogNiTo124 thanks for proposing help on the issue. Well, the issue is not a big deal. Just to wrap testing code that raises warning with pytest.warns. The steps are the following:

• take recent CI report, for example: https://travis-ci.org/github/pytorch/ignite/jobs/691837078
• find all messages about non catched warnings raised by ignite (like described in the issue message)
• find the test code responsible for raising warning. For example,
  
  
  
  • https://travis-ci.org/github/pytorch/ignite/jobs/691837078#L3602 =>
  
  
  • warning: tests/ignite/contrib/handlers/test_base_logger.py: 3 tests with warnings
  
  
  • test code is which seems to raise a warning is https://github.com/pytorch/ignite/blob/master/tests/ignite/contrib/handlers/test_base_logger.py#L120
  
  
• insert into test code:

with pytest.warns(...):
    # code that raises a warning

• rerun locally the test with

cd ignite
pytest tests/ignite/contrib/handlers/test_base_logger.py -vvv -k test_attach_on_custom_event

• go for another warning report

Does it sound good for you ?

[InCogNiTo124]

It sounds really doable. I'll open a PR by tommorow

[vfdev-5]

Maybe it would be better to fix the tests?

@InCogNiTo124 I'm sorry I do not understand what do you mean by fix the tests ?
In some sense, yes, we need to update tests by adding

with pytest.warns(...):
    # code that raises a warning

We do not need to modify ignite/ignite code, but only ignite/tests/ignite...

[InCogNiTo124]

Maybe it would be better to fix the tests?

@InCogNiTo124 I'm sorry I do not understand what do you mean by fix the tests ?
In some sense, yes, we need to update tests by adding

with pytest.warns(...):
    # code that raises a warning

We do not need to modify ignite/ignite code, but only ignite/tests/ignite...

Yes of course, I was confused for a bit, sorry. It's just that a lot of warnings are DeprecationWarnings and I was worried things will break if you ever up the dependencies.

Also, due to life, I don't think I'll be able to fix these tests until today. I hope next week is not too late?

[vfdev-5]

@InCogNiTo124 no worries. This issue is not related to coming release, so, it can wait.

[InCogNiTo124]

Although this issue is connected with tests, I found one line in the source connected to 66 warnings in 48 tests. ignite/metrics/accumulation.py:L57 creates a tensor with value 0.0 but dtype torch.long, which then throws a warning because pytorch is considering getting rid of implicit conversion with int().

Should I fix that line? The fix is trivial, but really effective, and without it I would have to manually fix a lot of lines.

[vfdev-5]

@InCogNiTo124 yes, sounds good !