Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8185 - Refactor test #8231

Open
wants to merge 32 commits into
base: dev
Choose a base branch
from

Conversation

garciadias
Copy link

@garciadias garciadias commented Nov 21, 2024

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_test coupled with the export_ckpt was 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 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_
@garciadias
Copy link
Author

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
Copy link
Member

ericspod commented Dec 2, 2024

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 garciadias closed this Dec 2, 2024
@garciadias garciadias reopened this Dec 2, 2024
@garciadias
Copy link
Author

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.

@ericspod
Copy link
Member

ericspod commented Dec 2, 2024

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need such a detailed folder structure for the tests, or would a simpler, flat structure at the top level suffice? Such as only apps/data/transforms...

Copy link
Author

@garciadias garciadias Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the word 'need' is unhelpful here. It's certainly not needed. None of this refactoring is needed. The project survived until now without these.
I think the question is how helpful it is to have the folder separation.
In my opinion, mirroring the structure of the code is the best way for developers to intuitively find the tests related to the portion of the code they are touching. For example, if you are changing something at monai/transforms/lazy/functional.py, it would be helpful to find up to the folder lazy in the tests. In that case, you would find 2 tests there. If you get all tests inside "transforms," you will have 304 test files to go through. Which I think is quite paralyzing.
I understand that some simplification could be beneficial to avoid having super-deep trees. I think @ericspod's proposition so far is good, but we can make it shallower if people find the current state too deep.

If testing is difficult or finding tests is hard, people will tend to ignore the tests or write repeated code.

However, I am not very familiar with the project's code. I am just starting to work on this, and I am happy to follow the team's guidance.

@garciadias
Copy link
Author

garciadias commented Dec 3, 2024

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?

@ericspod
Copy link
Member

ericspod commented Dec 5, 2024

Am I pushing too often? I see the whole pipeline is triggered. Should I accumulate some changes before pushing?

Thanks! It would be faster I think to put a number of changes together and trigger the tests less frequently. The contribution guide discusses running tests locally, that should help reducing load on the CI/CD system.

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.

Test Refactor
3 participants