Skip to content

Conversation

@jimchen90
Copy link
Contributor

This is related to #727.
Compared to this pull request and other test files (e.g. here), I remove the unittest.main function in the end and import unittest function in the first line in #727 .

@jimchen90 jimchen90 changed the title Unittest function in test. Unittest function in test Jun 23, 2020
@mthrok
Copy link
Contributor

mthrok commented Jun 23, 2020

Can you provide description of what value this PR brings?

@jimchen90
Copy link
Contributor Author

Can you provide description of what value this PR brings?

I want to separate this change with #727 and mention this difference in model test file when compare to other test files. It is for reference and it doesn't need to be merged.

@mthrok
Copy link
Contributor

mthrok commented Jun 23, 2020

it doesn't need to be merged.

if so can you close it?

@jimchen90 jimchen90 closed this Jun 23, 2020
@vincentqb
Copy link
Contributor

vincentqb commented Jun 23, 2020

Thanks for following up on comment and opening this @jimchen90 :) it'd be nice to understand the pros and cons of merging this or not, since we may want to apply this to all test files.

  • If we decide to add this, then all the test files should have it to maintain consistency. We'd then add that to other tests moving forward. One advantage is that users can run python test.py to run the tests locally. This does not require pytest, and, at this time, we prefer not depending on pytest due to internal systems.

  • If we decide to remove this, then we should remove this from all files eventually, and not add it moving forward. Users who want to run this using the command line need to use python -m pytest test.py. Other ways?

I'm ok to add it or not, but we should make an explicit decision so we can use that same decision moving forward :) @jimchen90 @mthrok thoughts?

@vincentqb vincentqb reopened this Jun 23, 2020
@mthrok
Copy link
Contributor

mthrok commented Jun 23, 2020

I am against adding the code for such a infinitesimal benefit.

  • Firstly, none of our CI system requires this.
  • Secondly, it is not clear why it has to be unittest.main that will be invoked when ran as regular python script. Why not another test runner suite?
  • Thirdly, there is an ongoing discussion on adopting pytest in PyTorch testing framework. If we add this code, we have to change that in the future.

Why do we want to increase the amount of code we have to maintain, which is not mission-critical, and we are not certain that it will be useful when there is a lot of alternative solutions, which is more powerful, such as simple pytest command invocation.

@vincentqb
Copy link
Contributor

  • Firstly, none of our CI system requires this.
  • Secondly, it is not clear why it has to be unittest.main that will be invoked when ran as regular python script. Why not another test runner suite?
  • Thirdly, there is an ongoing discussion on adopting pytest in PyTorch testing framework. If we add this code, we have to change that in the future.

Cool, I agree with (1) and (3). Let's not add it to files moving forward, and we can remove

if __name__ == "__main__":
    unittest.main()

from existing files. If pytorch decides to go with a particular framework, we'll consider it at that time. Glad we documented our decision on this :)

@vincentqb vincentqb closed this Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants