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

Align Engine.from_config with CLI behavior #2973

Merged
merged 15 commits into from
Feb 27, 2024
Merged

Align Engine.from_config with CLI behavior #2973

merged 15 commits into from
Feb 27, 2024

Conversation

harimkang
Copy link
Contributor

@harimkang harimkang commented Feb 23, 2024

Summary

  • Refactoring Engine.from_config() & CLI
  • Align Engine.from_config and CLI to have the same behavior as much as possible

The two examples below do the same thing.
API: from_config

from otx.engine import Engine

recipe_path = "src/otx/recipe/classification/multi_class_cls/otx_efficientnet_b0.yaml"
data_root = "/home/harimkan/workspace/repo/datasets/otx_v2_dataset/multiclass_classification/multiclass_CUB_medium"


engine = Engine.from_config(
    config_path=recipe_path, data_root=data_root, work_dir="otx-workspace-api", seed=1234
)

# Metric is now engine.train, engine.test argument, So we need to declare this separately.
from functools import partial
from torchmetrics.classification.accuracy import Accuracy

metric = partial(Accuracy, task="multiclass")
engine.train(
    deterministic=True,
    max_epochs=90,
    precision=16,
    metric=metric,
)

engine.test(metric=metric)

CLI command

otx train \
--config src/otx/recipe/classification/multi_class_cls/otx_efficientnet_b0.yaml \
--data_root /home/harimkan/workspace/repo/datasets/otx_v2_dataset/multiclass_classification/multiclass_CUB_medium \
--seed 1234 \
--deterministic True \
--work_dir otx-workspace-cli

Same Test results
image

How to test

Checklist

  • I have added unit tests to cover my changes.​
  • I have added integration tests to cover my changes.​
  • I have added e2e tests for validation.
  • 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) 2023 Intel Corporation
# SPDX-License-Identifier: Apache-2.0

@github-actions github-actions bot added the TEST Any changes in tests label Feb 23, 2024
@harimkang harimkang changed the title Aligning Engine.from_config with CLI behavior Align Engine.from_config with CLI behavior Feb 23, 2024
jaegukhyun
jaegukhyun previously approved these changes Feb 26, 2024
src/otx/cli/cli.py Show resolved Hide resolved
Copy link
Contributor

@sungmanc sungmanc left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, please check my comments

src/otx/cli/utils/jsonargparse.py Show resolved Hide resolved
src/otx/cli/cli.py Outdated Show resolved Hide resolved
src/otx/core/utils/cache.py Show resolved Hide resolved
src/otx/cli/cli.py Outdated Show resolved Hide resolved
src/otx/cli/cli.py Outdated Show resolved Hide resolved
Copy link
Contributor

@vinnamkim vinnamkim 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 know why you need this because it will show a different behavior from my expectation if an user executes the code not exactly as you provided such as

engine = Engine.from_config(
    config_path=recipe_path, data_root=data_root, work_dir="otx-workspace-api", seed=1234
)

***********************************************************
Execute some codes generating random variables,
so that change random number generator's inner states
***********************************************************

engine.train(
    deterministic=True,
    max_epochs=90,
    precision=16,
    metric=metric,
)

engine.test(metric=metric)

If you really need this, I think that it only happens if deterministic=True, I would recommend you to implement

class Engine:
    def train(self, deterministic=True):
          if deterministic:
              reset_random_number_generator()
              reinit_model_weights()
              reinit_data_sampler()
          train()

@harimkang
Copy link
Contributor Author

harimkang commented Feb 27, 2024

I don't know why you need this because it will show a different behavior from my expectation if an user executes the code not exactly as you provided such as

If you really need this, I think that it only happens if deterministic=True, I would recommend you to implement

class Engine:
    def train(self, deterministic=True):
          if deterministic:
              reset_random_number_generator()
              reinit_model_weights()
              reinit_data_sampler()
          train()

Yes you are right, we need to take care of this on the seed, deterministic side to get the same behavior for now (which probably doesn't look natural to the user).
I'll probably think about your suggestion in a future PR and take a look at it, thanks for the suggestion!

src/otx/core/utils/cache.py Show resolved Hide resolved
@harimkang harimkang merged commit 5725e46 into openvinotoolkit:v2 Feb 27, 2024
11 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.

5 participants