8185 - Refactor test #8231

garciadias · 2024-11-21T16:54:37Z

Description

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 tests folder and moved the files accordingly. I used some helper scripts, but the process required substantial manual intervention. When uncertain, I moved the tests to the integration folder 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.


### 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).
- [x] Quick tests passed locally by running `./runtests.sh --quick --unittests  --disttests`.

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_

tests/test_bundle_ckpt_export.py

This reverts commit bfc68a4.

This reverts commit e8d3e19.

…line_tests" This reverts commit d4b01e6.

for more information, see https://pre-commit.ci

garciadias · 2024-11-28T14:19:44Z

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

ericspod · 2024-12-02T00:04:24Z

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 array/dictionary directories for the transforms should have their contents moved to the parent, or some directories with one file should also have that file moved to the parent. It looks much better with more structure, I just don't want too deep a structure that is cumbersome itself. This is definitely in the right direction!

Good to hear you've come on board with us, we should meet in person and discuss things further. Thanks!

garciadias · 2024-12-02T10:40:41Z

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 array/dictionary directories for the transforms should have their contents moved to the parent, or some directories with one file should also have that file moved to the parent. It looks much better with more structure, I just don't want too deep a structure that is cumbersome itself. This is definitely in the right direction!

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?

tests/test_bundle_ckpt_export.py
tests/test_fl_monai_algo_dist.py
tests/test_handler_metrics_saver_dist.py
tests/test_integration_classification_2d.py
tests/test_integration_segmentation_3d.py

If these are all missing files you are referring to, I can confirm that git marked them as deleted, but they are present at:

tests/bundle/test_bundle_ckpt_export.py
tests/fl/client/monai_algo/test_fl_monai_algo_dist.py
tests/handlers/test_handler_metrics_saver_dist.py
tests/integration/test_integration_classification_2d.py
tests/integration/test_integration_segmentation_3d.py

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.
If possible, I would love to be included in any of the meetings you have.

Many thanks, Eric.

for more information, see https://pre-commit.ci

ericspod · 2024-12-02T17:25:28Z

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 array/dictionary directories for the transforms should have their contents moved to the parent, or some directories with one file should also have that file moved to the parent. It looks much better with more structure, I just don't want too deep a structure that is cumbersome itself. This is definitely in the right direction!
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?
* tests/test_bundle_ckpt_export.py

* tests/test_fl_monai_algo_dist.py

* tests/test_handler_metrics_saver_dist.py

* tests/test_integration_classification_2d.py

* tests/test_integration_segmentation_3d.py
If these are all missing files you are referring to, I can confirm that git marked them as deleted, but they are present at:
* tests/bundle/test_bundle_ckpt_export.py

* tests/fl/client/monai_algo/test_fl_monai_algo_dist.py

* tests/handlers/test_handler_metrics_saver_dist.py

* tests/integration/test_integration_classification_2d.py

* tests/integration/test_integration_segmentation_3d.py
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. If possible, I would love to be included in any of the meetings you have.

Many thanks, Eric.

Hi @garciadias There were 2 files mentioned as deleted in the "Files changed" section here, the second of these test_clip_intensity_percentilesd.py appears missing entirely. The conflicts appear resolved but there are other errors now to resolve. This looks like it's related to how we were excluding files in the past that assumed a flat structure that isn't there anymore. We can fix that for now but then think about improving that process later. Thanks!

tests/apps/reconstruction/test_complex_utils.py

garciadias · 2024-12-03T11:52:49Z

Hi @garciadias There were 2 files mentioned as deleted in the "Files changed" section here, the second of these test_clip_intensity_percentilesd.py appears missing entirely. The conflicts appear resolved but there are other errors now to resolve. This looks like it's related to how we were excluding files in the past that assumed a flat structure that isn't there anymore. We can fix that for now but then think about improving that process later. Thanks!

Thank you, @ericspod. I have now restored the test_clip_intensity_percentilesd.py file, and the other one 'missing', is at tests/bundle/test_bundle_ckpt_export.py.

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?

KumoLiu · 2025-02-03T16:06:06Z

runtests.sh

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

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.

Seems related to https://github.com/Project-MONAI/MONAI/blob/dev/tests/runner.py#L127

@garciadias This is one of those places that we need to do recursive glob searching like the other areas we discussed. The whole block mentioned here can be done as something like:

with PerfContext() as pc: # the files are searched from `tests/` folder, starting with `test_` cur_dir = os.path.dirname(__file__) files = glob.glob(os.path.join(cur_dir, "**", "test_*.py"), recursive=True) files = [f for f in files if re.match(args.pattern, f)] cases = [os.path.relpath(f, cur_dir).replace(os.sep, ".").replace(".py", "") for f in files] print(cases) tests = unittest.TestLoader().loadTestsFromNames(cases)

I think what you have in the current version if equivalent, I'm double checking myself in case we've missed something but then any other issues could be in runtests.sh as you mentioned.

Just noticed that it has been updated. Please ignore.

Yes, that was the problem. I think I have now fixed it. I can't find any coverage report for the most recent commits. Is that something I need to trigger?

Codecov used to post a link for things in PRs, but the page is always https://app.codecov.io/gh/Project-MONAI/MONAI I think.

The link for this PR however isn't so helpful at the moment: https://app.codecov.io/gh/Project-MONAI/MONAI/pull/8231

tests/integration/test_image_rw.py

KumoLiu · 2025-02-03T16:46:57Z

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
https://app.codecov.io/gh/Project-MONAI/MONAI/commit/d4a818fb9f070a4d53aa9ebadf618c486f71b3a8

garciadias · 2025-02-04T11:36:26Z

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 https://app.codecov.io/gh/Project-MONAI/MONAI/commit/d4a818fb9f070a4d53aa9ebadf618c486f71b3a8

I think it is related to the issue you pointed out on the unittest. I will work on fixing this.

…ndling

…t_scripts_fold

KumoLiu · 2025-02-07T10:07:48Z

/build

atbenmurray · 2025-02-07T15:16:41Z

I suggest that we add __init__.py to each of the test subfolders so that automated test discovery works for applications such as VSCode

for more information, see https://pre-commit.ci

…ests-refactor

ericspod · 2025-02-07T19:05:37Z

tests/CT_DICOM/__init__.py

Adding __init__.py files to directories which do not contain code leads to unexpected behaviour, I think some of your test fails are related to this. I'd remove this and any others added to pure data directories, but leave those in directories containing subdirectories with code.

Signed-off-by: R. Garcia-Dias <[email protected]>

garciadias added 5 commits November 20, 2024 18:07

Eliminates code repetition

075ec11

Replace relative import with absolute

e8d3e19

Style fix

bfc68a4

Merge branch 'dev' into 8185-tests-refactor

10eb2af

ericspod reviewed Nov 22, 2024

View reviewed changes

tests/test_bundle_ckpt_export.py Outdated Show resolved Hide resolved

ericspod reviewed Nov 22, 2024

View reviewed changes

tests/test_bundle_ckpt_export.py Outdated Show resolved Hide resolved

garciadias added 3 commits November 23, 2024 13:37

Revert "Style fix"

55fcd22

This reverts commit bfc68a4.

Revert "Replace relative import with absolute"

ba16743

This reverts commit e8d3e19.

Revert "Directly tests export_ckpt function instead of using command_…

1667eb7

…line_tests" This reverts commit d4b01e6.

garciadias force-pushed the 8185-tests-refactor branch from 3e0fe76 to 1667eb7 Compare November 24, 2024 16:15

pre-commit-ci bot and others added 4 commits November 24, 2024 16:15

[pre-commit.ci] auto fixes from pre-commit.com hooks

702e740

for more information, see https://pre-commit.ci

moves tests to their corresponding folder

9a2c2f4

replace relative imports with absolute import

f2cf158

Change locating integrations

b3fd8e9

Corrects the download path for integration tests

114fc72

garciadias closed this Dec 2, 2024

garciadias reopened this Dec 2, 2024

garciadias and others added 5 commits December 2, 2024 11:26

Move array/* and dictionary/* files to parent directory

626e050

Move single files on their parent folders

4c6b59c

Merge remote-tracking branch 'upstream/dev' into dev

050e71b

Merge branch 'dev' into 8185-tests-refactor

7d68938

[pre-commit.ci] auto fixes from pre-commit.com hooks

3508211

for more information, see https://pre-commit.ci

KumoLiu reviewed Dec 3, 2024

View reviewed changes

tests/apps/reconstruction/test_complex_utils.py Outdated Show resolved Hide resolved

Recovers deleted file

2d55074

KumoLiu reviewed Feb 3, 2025

View reviewed changes

Corrects _dist.py discovery on runtests.sh

8d4a657

KumoLiu reviewed Feb 3, 2025

View reviewed changes

tests/integration/test_image_rw.py Outdated Show resolved Hide resolved

garciadias added 11 commits February 4, 2025 14:39

Fix -u test discovery in runner.py using pathlib for improved path ha…

30a1e63

…ndling

corrects the excluded tests printing

8ecdd16

Autofix

1e281a6

Fixes testing_data paths

fbbcafc

Fixes more wrong paths

e7e48ae

Remove debugging breakpoint from runner.py

63b73d9

move test_image_rw.py to tests/data/

70966c0

Fix test paths in test_bundle_ckpt_export.py

48b4ef2

autofix

4b07a2d

Fixes tests.integration.test_integration_bundle_run TestBundleRun.tes…

c80df45

…t_scripts_fold

fixes paths on tests.networks.test_bundle_onnx_export

9f3474e

fix text discovery

2481afb

garciadias and others added 7 commits February 7, 2025 15:21

Remove nested folders

21132b5

Add init files to every tests subfolder

35e3036

Adds license to all __init__.py files

7247555

[pre-commit.ci] auto fixes from pre-commit.com hooks

beef622

for more information, see https://pre-commit.ci

autofix

9bb9cec

Merge remote-tracking branch 'origin/8185-tests-refactor' into 8185-t…

71793c8

…ests-refactor

fixes paths on moved files

1a6b9b3

ericspod reviewed Feb 7, 2025

View reviewed changes

garciadias added 3 commits February 8, 2025 11:42

removes __init__.py from data folders

95d1d15

Signed-off-by: R. Garcia-Dias <[email protected]>

removes __init__.py from data folders

59f75bf

Signed-off-by: R. Garcia-Dias <[email protected]>

removes __init__.py from data folders

d43a30a

Signed-off-by: R. Garcia-Dias <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8185 - Refactor test #8231

8185 - Refactor test #8231

garciadias commented Nov 21, 2024 •

edited

Loading

garciadias commented Nov 28, 2024

ericspod commented Dec 2, 2024

garciadias commented Dec 2, 2024

ericspod commented Dec 2, 2024

Missing files:

Conflicts:

Folder depth:

garciadias commented Dec 3, 2024 •

edited

Loading

KumoLiu Feb 3, 2025

garciadias Feb 3, 2025

KumoLiu Feb 7, 2025

ericspod Feb 7, 2025 •

edited

Loading

KumoLiu Feb 7, 2025

garciadias Feb 7, 2025

garciadias Feb 7, 2025

ericspod Feb 7, 2025

ericspod Feb 7, 2025

KumoLiu commented Feb 3, 2025

garciadias commented Feb 4, 2025

KumoLiu commented Feb 7, 2025

atbenmurray commented Feb 7, 2025 •

edited

Loading

ericspod Feb 7, 2025 •

edited

Loading

8185 - Refactor test #8231

Are you sure you want to change the base?

8185 - Refactor test #8231

Conversation

garciadias commented Nov 21, 2024 • edited Loading

Description

Reorganize tests

garciadias commented Nov 28, 2024

ericspod commented Dec 2, 2024

garciadias commented Dec 2, 2024

Missing files:

Conflicts:

Folder depth:

ericspod commented Dec 2, 2024

Missing files:

Conflicts:

Folder depth:

garciadias commented Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericspod Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KumoLiu commented Feb 3, 2025

garciadias commented Feb 4, 2025

KumoLiu commented Feb 7, 2025

atbenmurray commented Feb 7, 2025 • edited Loading

ericspod Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

garciadias commented Nov 21, 2024 •

edited

Loading

garciadias commented Dec 3, 2024 •

edited

Loading

ericspod Feb 7, 2025 •

edited

Loading

atbenmurray commented Feb 7, 2025 •

edited

Loading

ericspod Feb 7, 2025 •

edited

Loading