8185 - Refactor test #8231
8185 - Refactor test #8231
Conversation
Performance Before: 94.81s call tests/test_bundle_ckpt_export.py::TestCKPTExport::test_default_value_1_model 20.95s call tests/test_bundle_ckpt_export.py::TestCKPTExport::test_default_value_0_ 15.26s call tests/test_bundle_ckpt_export.py::TestCKPTExport::test_export_2_model 14.86s call tests/test_bundle_ckpt_export.py::TestCKPTExport::test_default_value_2_model 14.55s call tests/test_bundle_ckpt_export.py::TestCKPTExport::test_export_1_model 14.28s call tests/test_bundle_ckpt_export.py::TestCKPTExport::test_export_0_ Performance after: 1.62s call tests/test_bundle_ckpt_export.py::TestCKPTExport::test_ckpt_export_2_model 1.25s call tests/test_bundle_ckpt_export.py::TestCKPTExport::test_ckpt_export_default_2_model 0.64s call tests/test_bundle_ckpt_export.py::TestCKPTExport::test_ckpt_export_0_ 0.57s call tests/test_bundle_ckpt_export.py::TestCKPTExport::test_ckpt_export_1_model 0.57s call tests/test_bundle_ckpt_export.py::TestCKPTExport::test_ckpt_export_default_1_model 0.55s call tests/test_bundle_ckpt_export.py::TestCKPTExport::test_ckpt_export_default_0_ 0.01s setup tests/test_bundle_ckpt_export.py::TestCKPTExport::test_ckpt_export_0_
garciadias
force-pushed
the
8185-tests-refactor
branch
from
3e0fe76 to
1667eb7
Compare
|
Hi @ericspod, Just for context, last week, I started working in Jorge Cardoso's team at KCL, so I am still getting access to the cluster and waiting for a new laptop with a dedicated GPU. I am currently working on a laptop with integrated graphics, which is not very powerful. So, it has been hard for me to tackle the slow tests since they would run SUPER slow for me. This is why I moved to reorganise the tests. Since this obscures any further changes in these files on the tracking records, I suggest you review this part first, and I create a new pull request for the other points in your issue. Please let me know what you think. Many thanks |
Hi @garciadias I see that the tests have been moved which is good, but there's a few files that seem to have gone missing and some conflicts that need to be resolved. I'm sure you've kept what you were working on earlier so we can come back to that when you're ready. These changes are going to represent a significant change from what's in the tests folder so we'll have to discuss with other developers more. We discussed it briefly at the Core meeting but we should again, what I can suggest now is that the directory structure doesn't have to be so deep as you have it, so perhaps the Good to hear you've come on board with us, we should meet in person and discuss things further. Thanks! |
Hi @ericspod, thank you very much for reviewing this and for the welcoming regards. Missing files:When you say there are some missing files, are you referring to these file files?
If these are all missing files you are referring to, I can confirm that git marked them as deleted, but they are present at:
I don't understand why these were flagged as deleted. Conflicts:I will do my best to merge the current dev branch to this and keep solving the conflicts while we wait for approval of this change. Folder depth:Fair enough, I will move them. I am happy to keep adjusting until we are satisfied with the result. Please keep me posted on the conversation with the core group. Many thanks, Eric. |
for more information, see https://pre-commit.ci
Hi @garciadias There were 2 files mentioned as deleted in the "Files changed" section here, the second of these |
Thank you, @ericspod. I have now restored the I will be working on solving the other issues. Am I pushing too often? I see the whole pipeline is triggered. Should I accumulate some changes before pushing? |
Fixes Project-MONAI#8298 ### Types of changes <!--- Put an `x` in all the boxes that apply, and remove the not applicable items --> - [x] Non-breaking change (fix or new feature that would not break existing functionality). - [ ] Breaking change (fix or new feature that would cause existing functionality to change). - [ ] New tests added to cover the changes. - [ ] Integration tests passed locally by running `./runtests.sh -f -u --net --coverage`. - [ ] Quick tests passed locally by running `./runtests.sh --quick --unittests --disttests`. - [ ] In-line docstrings updated. - [ ] Documentation updated, tested `make html` command in the `docs/` folder. --------- Signed-off-by: YunLiu <[email protected]> Co-authored-by: Eric Kerfoot <[email protected]>
I, R. Garcia-Dias <[email protected]>, hereby add my Signed-off-by to this commit: 4af739c I, R. Garcia-Dias <[email protected]>, hereby add my Signed-off-by to this commit: 19b2fdf I, R. Garcia-Dias <[email protected]>, hereby add my Signed-off-by to this commit: 7a34f01 I, R. Garcia-Dias <[email protected]>, hereby add my Signed-off-by to this commit: 67c0521 I, R. Garcia-Dias <[email protected]>, hereby add my Signed-off-by to this commit: dc7b39b I, R. Garcia-Dias <[email protected]>, hereby add my Signed-off-by to this commit: e789ddd I, R. Garcia-Dias <[email protected]>, hereby add my Signed-off-by to this commit: 931f01d Signed-off-by: R. Garcia-Dias <[email protected]>
for more information, see https://pre-commit.ci
I, R. Garcia-Dias <[email protected]>, hereby add my Signed-off-by to this commit: 4af739c I, R. Garcia-Dias <[email protected]>, hereby add my Signed-off-by to this commit: 19b2fdf I, R. Garcia-Dias <[email protected]>, hereby add my Signed-off-by to this commit: 7a34f01 I, R. Garcia-Dias <[email protected]>, hereby add my Signed-off-by to this commit: 67c0521 I, R. Garcia-Dias <[email protected]>, hereby add my Signed-off-by to this commit: dc7b39b I, R. Garcia-Dias <[email protected]>, hereby add my Signed-off-by to this commit: e789ddd I, R. Garcia-Dias <[email protected]>, hereby add my Signed-off-by to this commit: 931f01d I, R. Garcia-Dias <[email protected]>, hereby add my Signed-off-by to this commit: 069b07c I, R. Garcia-Dias <[email protected]>, hereby add my Signed-off-by to this commit: a4a0baa I, R. Garcia-Dias <[email protected]>, hereby add my Signed-off-by to this commit: 34627e8 I, R. Garcia-Dias <[email protected]>, hereby add my Signed-off-by to this commit: 08cd2ac I, R. Garcia-Dias <[email protected]>, hereby add my Signed-off-by to this commit: 74d0481 I, R. Garcia-Dias <[email protected]>, hereby add my Signed-off-by to this commit: 1091266 I, R. Garcia-Dias <[email protected]>, hereby add my Signed-off-by to this commit: 58e9e3b I, R. Garcia-Dias <[email protected]>, hereby add my Signed-off-by to this commit: f8d9bcf I, R. Garcia-Dias <[email protected]>, hereby add my Signed-off-by to this commit: cf1025a Signed-off-by: R. Garcia-Dias <[email protected]>
Hi @ericspod, I noticed the CI check that is not passing is related to the commit signing. I tried to solve it by following the guides from the bot on creating new commits. However, it doesn't seem to solve the issue. Should I do something about it or it doesn't matter? |
It gets confused sometimes if you change your name or email address in your git settings, I just set it to pass. Make sure your |
…n requirements-dev.txt to avoid conflicts with black
|
The pre-commit.ci failure is a version issue with pycln, you can change the file to use version 1.5.0 to fix this. For the code format tests, be sure you have the most recent versions of isort and black installed and then run |
Thank you, @ericspod. I was going crazy with testing multiple combinations of black and isort. I will try to fix the pycln version. I assume you are referring to this line on this file. |
Yes that's the line. I'm noticing some weird stuff with isort and black versions as well with another PR, I'm not sure what's causing it but we may have to fix versions for those to get this PR through then come back to fixing tests. |
|
and I meant version 2.5.0 sorry. |
|
In |
Co-authored-by: Eric Kerfoot <[email protected]> Signed-off-by: Rafael Garcia-Dias <[email protected]>
I suspect you meant 2.5.0, I should have asked. Yes, I did fix the |
…ack modifications
|
@KumoLiu please trigger blossom when you can. We have had to restrict the versions of Pytorch (since v1.6.0 showed up in the Pytorch list of CPU packages for some reason) and isort (because v6.0 doesn't agree with black), so long as blossom's environment will ensure this we should be good. |
|
/build |
Looks like some torch cuda init issue. Let me try to trigger it again. |
|
/build |
|
Still Failed. Can we create another PR that only restricts the Pytorch version? I don't know why it failed. Log
|
|
Oh, I got the point. I may need to modify the test files in the blossom. |
Fixes #8185
Description
Speed up slow-running tests
Starts the work on test refactoring as described in issue #8185.
This is an initial pull request aimed at aligning the changes that I intend to make so the team can redirect me if I am going in the wrong direction.
I started by identifying the slowest tests in the stack which were not flagged as integration tests or downloads.
Starting with the slowest test on this criteria, I eliminated code duplications and rewritten the code for clarity.
Initially, I assumed that the end-to-end test of the
command_line_testcoupled with theexport_ckptwas redundant with the test of the independent behaviours and interfaces. Based on @ericspod's advice, I have reverted the changes and will keep the original integration of these two functions.Reorganize tests
I have looked at the imports in each test file and the test title to identify which files were being tested. I mirrored the file structure of MONAI on the
testsfolder and moved the files accordingly. I used some helper scripts, but the process required substantial manual intervention. When uncertain, I moved the tests to theintegrationfolder since the confusion always involved many imports, and I could not find clarity from the test name.Please review the integration folder carefully, which is the one that I feel the least confident about.