-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
base: dev
Are you sure you want to change the base?
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_
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 |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
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? |
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. |
for more information, see https://pre-commit.ci
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 theexport_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 theintegration
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.