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

[FEATURE] Add train-type parameter to otx train #1874

Merged
merged 2 commits into from
Mar 14, 2023
Merged

Conversation

sovrasov
Copy link
Contributor

@sovrasov sovrasov commented Mar 9, 2023

Currently, to enable SSL training we have to execute oxt build at the first step.
At the same time, otx train allows passing --unlabeled-data-roots, which causes a failure if otx build hasn't been executed before.
Therefore, CLI of otx train is contradictive and incomplete. This PR brings back missing ability to launch different training types right from otx train CLI.

This contradiction is caused by mixing two paradigms for launching a training: pre-configuration of a workspace and passing all the required arguments via CLI. The possible solutions are:

  • restricting of otx train CLI.
  • maintaining otx train CLI as a standalone and self-containing tool for configuring and launching trainings.

This PR follows the second option, but the first one deserves discussion as well.

@sovrasov sovrasov requested a review from a team as a code owner March 9, 2023 14:23
@sovrasov sovrasov changed the title Add train-type parameter to otx train [FEATURE] Add train-type parameter to otx train Mar 9, 2023
@github-actions github-actions bot added CLI Any changes in OTE CLI DOC Improvements or additions to documentation labels Mar 9, 2023
@sovrasov sovrasov added this to the 1.1.0 milestone Mar 9, 2023
@sovrasov sovrasov force-pushed the vs/add_train_type branch 2 times, most recently from 6a8a893 to ac734b7 Compare March 9, 2023 14:28
@sovrasov sovrasov requested a review from goodsong81 March 9, 2023 14:29
@codecov-commenter
Copy link

codecov-commenter commented Mar 9, 2023

Codecov Report

Patch coverage: 69.44% and no project coverage change.

Comparison is base (0e4a8b7) 80.54% compared to head (4b2ec68) 80.55%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1874   +/-   ##
========================================
  Coverage    80.54%   80.55%           
========================================
  Files          484      483    -1     
  Lines        33005    32968   -37     
========================================
- Hits         26585    26557   -28     
+ Misses        6420     6411    -9     
Impacted Files Coverage Δ
...hms/classification/adapters/mmcls/data/datasets.py 76.86% <ø> (ø)
...odels/heads/custom_hierarchical_linear_cls_head.py 82.10% <ø> (ø)
...s/heads/custom_hierarchical_non_linear_cls_head.py 83.65% <ø> (ø)
otx/algorithms/common/tasks/training_base.py 53.27% <16.66%> (+0.03%) ⬆️
...dapters/openvino/model_wrappers/openvino_models.py 74.57% <50.00%> (ø)
otx/algorithms/detection/tasks/train.py 82.11% <50.00%> (ø)
otx/algorithms/detection/tasks/inference.py 73.56% <66.66%> (+0.13%) ⬆️
otx/algorithms/segmentation/tasks/inference.py 89.04% <66.66%> (+1.22%) ⬆️
otx/algorithms/classification/tasks/inference.py 86.84% <80.00%> (+0.61%) ⬆️
otx/algorithms/classification/utils/cls_utils.py 100.00% <100.00%> (ø)
... and 6 more

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

goodsong81
goodsong81 previously approved these changes Mar 10, 2023
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. @harimkang Could you have a look?

@sungmanc
Copy link
Contributor

I'm not sure why we should use otx build for SSL training. Users could use otx cli if they give train_params.
We currently use params --algo_backend.train_type to enable the different train types.

Example for SemiSL:

class TestToolsMultiClassSemiSLClassification:

Example for SelfSL:

@harimkang
Copy link
Contributor

I'm not sure why we should use otx build for SSL training. Users could use otx cli if they give train_params.

We currently use params --algo_backend.train_type to enable the different train types.

Example for SemiSL:

class TestToolsMultiClassSemiSLClassification:

Example for SelfSL:

I agree to some extent, but vlad seems to want to improve the inconvenience. However, there can be conflicts between the two methods(use train_type / use params), and it seems that this could cause problems somewhere. (maybe not..?)

@sovrasov
Copy link
Contributor Author

sovrasov commented Mar 13, 2023

if self.mode in ("build", "train") and self.ar

_get_train_type method has a clear logic to extract training type from template & parameters: according to the code --train_type (when it's specified) will be used if params --algo_backend.train_type is not specified, so I don't expect any unpredictable behavior here.

@sovrasov
Copy link
Contributor Author

I'm not sure why we should use otx build for SSL training. Users could use otx cli if they give train_params. We currently use params --algo_backend.train_type to enable the different train types.

Example for SemiSL:

class TestToolsMultiClassSemiSLClassification:

Example for SelfSL:

params --... is also a correct way, but it's not documented and hard to discover (since otx train --help doesn't reveal that's hidden under the params section).

@sungmanc
Copy link
Contributor

sungmanc commented Mar 14, 2023

After this PR is merged, we need to consider eliminating params --algo_backend.train_type from the configuration.

@sovrasov sovrasov merged commit 4385187 into develop Mar 14, 2023
@sovrasov sovrasov deleted the vs/add_train_type branch March 14, 2023 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI Any changes in OTE CLI DOC Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants