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? |
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. |
Head branch was pushed to by a user without write access
|
/build |
There was a problem hiding this comment.
unittest logic didn't update here.
Now:
[2025-02-03T15:33:49.029Z] With file pattern: '^(?!test_integration).*(?<!_dist)$'
[2025-02-03T15:33:49.029Z] monai test runner: excluding tests.test_call_dist
[2025-02-03T15:33:49.029Z] monai test runner: excluding tests.test_timedcall_dist
[2025-02-03T15:33:49.029Z] ['tests.test_rand_torchio', 'tests.test_query_memory', 'tests.test_utils', 'tests.test_rand_torchiod', 'tests.test_masked_autoencoder_vit', 'tests.test_torchiod', 'tests.test_torchio', 'tests.test_print_info', 'tests.test_set_visible_devices', 'tests.test_auto3dseg']
[2025-02-03T15:33:49.029Z] time to discover tests: 1.4007233679294586s, total cases: 66.
Previous:
time to discover tests: 10.140145242214203s, total cases: 15864.
BTW, didn't check other tests.
also cc @ericspod
There was a problem hiding this comment.
I am looking into the runtests.py file and trying to solve this. This regular expression seems to be filtering the tests, which is ok, but it is probably looking only on the root folder of the tests.
tests/integration/test_image_rw.py
Outdated
There was a problem hiding this comment.
Seems this file has been put in the wrong folder, should not be integration.
There was a problem hiding this comment.
I think this should go in the data directory?
|
It appears that there has been a significant drop in code coverage. Do you have any insights into why this might be happening, and should we be concerned about it? @ericspod @garciadias |
I think it is related to the issue you pointed out on the unittest. I will work on fixing this. |
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.