-
Notifications
You must be signed in to change notification settings - Fork 446
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
Add Simple Integration test (CLI - 'otx train') #2695
Conversation
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.
LGTM
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.
In general, LGTM. But it might be worth extending the tests by 2 epochs instead of 1 to make sure that any processes between epochs also work.
What do you think?
I think it's a good idea, but for now I'll merge this PR for another task model recipe integration test and create a new PR after we discuss it together. |
process = await asyncio.create_subprocess_exec( | ||
*cmd, | ||
stdout=asyncio.subprocess.PIPE, | ||
stderr=asyncio.subprocess.PIPE, | ||
env=environ, | ||
**kwargs, | ||
) |
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 do not want to spawn a separate process for the integration test. This is because it makes a developer harder to debug if a problem is found during the integration test. Please replace it with executing the CLI Python function in the main process.
"trainer.max_epochs=1", | ||
"trainer.check_val_every_n_epoch=1", | ||
"trainer=auto", |
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.
Rather than manually overriding at the variable level, consider to override this https://github.com/openvinotoolkit/training_extensions/blob/v2/src/otx/config/debug/default.yaml
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.
Thanks for the comment. I will create a PR to reflect your comments. I have a question regarding this comment. I want to create a config file called debug/intg_test.yaml
and want to apply it to test.
Can you tell me what args should be added to override the config in this debug folder in the cli?
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.
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.
Thank you. I'll give it a try.
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.
Why is this needed?
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 wanted to make sure that CI would work whether the test was using cpu or gpu, so I thought using the auto
provided by lightning for the test would be appropriate. What do you think? This too can be modified as per your suggestion.
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.
Is that possible by giving trainer=cpu
or trainer=gpu
explicitly outside? If you set it as auto
, then it seems not traceable which device we used for the tests.
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.
For test, yes i understand, I'm going to modify it in intg-test to gpu for now. Do you want to remove this file?
I guess auto
might be a good option for the user as well.
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.
Then, let it leave as is. However, the decision for which device will be used should be explicit in the tests. This is what I only concern.
@vinnamkim I create next PR #2701 |
Summary
tests/assets/classification_dataset
trainer.device=auto
settingHow to test
TOX test
tox -vv -e integration-test
Local Test
python -m pytest tests/integration/cli/test_cli.py
Checklist
License
Feel free to contact the maintainers if that's a concern.