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

Add '--test-only' flag in perf test #3746

Merged
merged 10 commits into from
Jul 17, 2024

Conversation

eunwoosh
Copy link
Contributor

@eunwoosh eunwoosh commented Jul 16, 2024

Summary

This PR adds --test-only flags to perf test. When it's set, perf test copies all necessary files from resume directory and executes only otx test with specified operation. If files can't be found from resume directory, necessary operations will be executed automatically.
Additionally, bug that error is raised when setting device to cpu is fixed.

How to test

Checklist

  • I have added unit tests to cover my changes.​
  • I have added integration tests to cover my changes.​
  • I have ran e2e tests and there is no issues.
  • I have added the description of my changes into CHANGELOG in my target branch (e.g., CHANGELOG in develop).​
  • I have updated the documentation in my target branch accordingly (e.g., documentation in develop).
  • I have linked related issues.

License

  • I submit my code changes under the same Apache License that covers the project.
    Feel free to contact the maintainers if that's a concern.
  • I have updated the license header for each file (see an example below).
# Copyright (C) 2024 Intel Corporation
# SPDX-License-Identifier: Apache-2.0

@github-actions github-actions bot added the TEST Any changes in tests label Jul 16, 2024
@eunwoosh eunwoosh changed the title Perf test resume from Update resume feature in perf test Jul 16, 2024
Copy link
Contributor

@goodsong81 goodsong81 left a comment

Choose a reason for hiding this comment

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

Isn't it a little bit misleading not executing otx export when specifying --resume-from export? It sounds like rather --resume-after to me.
(Not saying that it should be renamed to "after", thouth)

@eunwoosh
Copy link
Contributor Author

Isn't it a little bit misleading not executing otx export when specifying --resume-from export? It sounds like rather --resume-after to me. (Not saying that it should be renamed to "after", thouth)

you're right, but it's hard to find proper name. So far, --resume-from is best candidate, I think. I'll think more about that.

Copy link
Contributor

@kprokofi kprokofi left a comment

Choose a reason for hiding this comment

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

I don't understand, why should we add second option with resume_from. Why only one resume is not enough to continue training? Let's imagine we have some workspace with already completed train stage for small datasets. Can we just write: pytest test_*.py --work-dir /path/to/it --eval-upto optimize --resume. After that I would expect that export -> optimize on small datasets will be executed

It looks complicated for me with two resumes

@eunwoosh
Copy link
Contributor Author

I don't understand, why should we add second option with resume_from. Why only one resume is not enough to continue training? Let's imagine we have some workspace with already completed train stage for small datasets. Can we just write: pytest test_*.py --work-dir /path/to/it --eval-upto optimize --resume. After that I would expect that export -> optimize on small datasets will be executed

It looks complicated for me with two resumes

It's for saving more time when running huge number of models. Current perf test resume feature just skips training part. So, if user uses --resume-from, then perf test starts from otx test after training. Of course, skipping training part can save a lot of time, but sometimes skipping additional parts is preferred. For examples, recently I tried to benchmark score using all OTX models' optimized OV IR. With current resume feature, I need to run otx test 15 times using trained model, OV IR and quantized OV IR for every model (repeat 5 x 3 otx test). In this case, skipping otx test w/ trained model and OV IR can save a lot of time. And it can be beneficial when using environment w/o any acclerator where otx test w/ trained model spends lots of time.

@goodsong81
Copy link
Contributor

I don't understand, why should we add second option with resume_from. Why only one resume is not enough to continue training? Let's imagine we have some workspace with already completed train stage for small datasets. Can we just write: pytest test_*.py --work-dir /path/to/it --eval-upto optimize --resume. After that I would expect that export -> optimize on small datasets will be executed
It looks complicated for me with two resumes

It's for saving more time when running huge number of models. Current perf test resume feature just skips training part. So, if user uses --resume-from, then perf test starts from otx test after training. Of course, skipping training part can save a lot of time, but sometimes skipping additional parts is preferred. For examples, recently I tried to benchmark score using all OTX models' optimized OV IR. With current resume feature, I need to run otx test 15 times using trained model, OV IR and quantized OV IR for every model (repeat 5 x 3 otx test). In this case, skipping otx test w/ trained model and OV IR can save a lot of time. And it can be beneficial when using environment w/o any acclerator where otx test w/ trained model spends lots of time.

I think @kprokofi's comment makes sense. Isn't it some kind of test-only feature that you really need?
Why don't you add some flag like "--test train|export|optimize|all"?

@eunwoosh
Copy link
Contributor Author

eunwoosh commented Jul 17, 2024

I understand what you said. I'll revert "--resume" to "--resume-from" and change "--resume-from" to "--test-only".

@eunwoosh eunwoosh changed the title Update resume feature in perf test Add '--test-only' flag in perf test Jul 17, 2024
Copy link
Contributor

@goodsong81 goodsong81 left a comment

Choose a reason for hiding this comment

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

LGTM

@eunwoosh eunwoosh merged commit 9b59082 into openvinotoolkit:develop Jul 17, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TEST Any changes in tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants