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 Anomalib v1 tasks to OTX v2 #2902

Merged
merged 31 commits into from
Feb 28, 2024

Conversation

ashwinvaidya17
Copy link
Collaborator

@ashwinvaidya17 ashwinvaidya17 commented Feb 8, 2024

Summary

Remaining

  • Some files can be refactored and moved to separate folder to make them more readable
  • Performance numbers are lower than Anomalib. Need to investigate the cause.

How to test

import torch
from torchvision.transforms.v2 import Normalize, ToDtype

from otx.algo.anomaly.padim import Padim
from otx.algo.anomaly.stfpm import STFPM
from otx.core.data.transform_libs.torchvision import PadtoSquare, ResizetoLongestEdge
from otx.core.model.entity.anomaly import OTXAnomalyModel, OVAnomalyModel
from otx.core.types.export import OTXExportFormatType
from otx.core.types.task import OTXTaskType
from otx.data.anomaly import AnomalyDataModule
from otx.engine import Engine

transforms = [
    ResizetoLongestEdge(256, antialias=True),
    PadtoSquare(),
    ToDtype(torch.float32),
    Normalize(mean=[123.675, 116.28, 103.53], std=[58.395, 57.12, 57.375]),
]
model = Padim(input_size=(256, 256), layers=["layer1", "layer2", "layer3"], backbone="resnet18")
task = OTXTaskType.ANOMALY_CLASSIFICATION
engine = Engine(
        datamodule=datamodule,
        task=task,
        model=model,
        val_check_interval=1.0,
        num_sanity_val_steps=0,
    )
engine.train(max_epochs=1)
engine.test()
# OpenVINO
engine.export(export_format=OTXExportFormatType.OPENVINO)
model = OVAnomalyModel(
  model_name="otx-workspace/exported_model.xml",
  async_inference=True,
  use_throughput_mode=False,
)

engine = Engine(
    model=model,
    datamodule=datamodule,
    task=task,
)
engine.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

Copy link
Contributor

@harimkang harimkang 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 doing this, I have a few comments.

src/otx/core/data/dataset/anomaly/dataset.py Outdated Show resolved Hide resolved
src/otx/core/model/entity/anomaly.py Outdated Show resolved Hide resolved
src/otx/core/model/module/anomaly.py Outdated Show resolved Hide resolved
src/otx/core/model/module/anomaly.py Outdated Show resolved Hide resolved
src/otx/recipe/anomaly/openvino.yaml Outdated Show resolved Hide resolved
@vinnamkim vinnamkim mentioned this pull request Feb 15, 2024
8 tasks
src/otx/algo/anomaly/padim.py Outdated Show resolved Hide resolved
src/otx/core/model/module/anomaly.py Outdated Show resolved Hide resolved
src/otx/core/model/module/anomaly.py Outdated Show resolved Hide resolved
@ashwinvaidya17
Copy link
Collaborator Author

I've pushed new changes. Not sure if this helps for now but maybe with the OTXModel refactor it might become better. But it would be nice to get feedback from you all.
I still have to extend changes to Anomaly OpenVINO inference.

@harimkang
Copy link
Contributor

I've pushed new changes. Not sure if this helps for now but maybe with the OTXModel refactor it might become better. But it would be nice to get feedback from you all. I still have to extend changes to Anomaly OpenVINO inference.

If possible, could you please separate IR-related functionality into the next PR?

@ashwinvaidya17
Copy link
Collaborator Author

I've pushed new changes. Not sure if this helps for now but maybe with the OTXModel refactor it might become better. But it would be nice to get feedback from you all. I still have to extend changes to Anomaly OpenVINO inference.

If possible, could you please separate IR-related functionality into the next PR?

Do you want me to remove the export stuff from this PR?

@harimkang
Copy link
Contributor

harimkang commented Feb 21, 2024

I've pushed new changes. Not sure if this helps for now but maybe with the OTXModel refactor it might become better. But it would be nice to get feedback from you all. I still have to extend changes to Anomaly OpenVINO inference.

If possible, could you please separate IR-related functionality into the next PR?

Do you want me to remove the export stuff from this PR?

I'm not saying you have to do that, but if you think the IR is going to take a long time. It seems like a good idea to separate things that are going to take a long time. (It doesn't look good to have PRs open for too long.)

If you do separate them, you can skip the IR-related integration tests for now.

Copy link
Contributor

@samet-akcay samet-akcay left a comment

Choose a reason for hiding this comment

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

@ashwinvaidya17, thanks a lot for the great effort! Much appreciated.

I've added some minor comments, and some questions.

src/otx/algo/anomaly/padim.py Show resolved Hide resolved
src/otx/algo/anomaly/stfpm.py Outdated Show resolved Hide resolved
src/otx/algo/anomaly/stfpm.py Outdated Show resolved Hide resolved
src/otx/core/data/dataset/anomaly/dataset.py Outdated Show resolved Hide resolved
src/otx/algo/anomaly/padim.py Show resolved Hide resolved
src/otx/core/model/module/anomaly/anomaly_lightning.py Outdated Show resolved Hide resolved
src/otx/data/__init__.py Show resolved Hide resolved
src/otx/data/anomaly/__init__.py Show resolved Hide resolved
src/otx/data/anomaly/anomaly.py Show resolved Hide resolved
src/otx/recipe/anomaly/anomaly_classification/stfpm.yaml Outdated Show resolved Hide resolved
src/otx/engine/engine.py Outdated Show resolved Hide resolved
Remove OpenVINO model
Add anomalib install to tox

Signed-off-by: Ashwin Vaidya <ashwinnitinvaidya@gmail.com>
Signed-off-by: Ashwin Vaidya <ashwinnitinvaidya@gmail.com>
@github-actions github-actions bot added the BUILD label Feb 26, 2024
@ashwinvaidya17
Copy link
Collaborator Author

Can we skip python 3.9 checks for anomaly task? Anomalib v1 supports 3.10 and above

samet-akcay
samet-akcay previously approved these changes Feb 26, 2024
Copy link
Contributor

@samet-akcay samet-akcay 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 effort!

src/otx/algo/anomaly/padim.py Show resolved Hide resolved
@harimkang
Copy link
Contributor

Can we skip python 3.9 checks for anomaly task? Anomalib v1 supports 3.10 and above

I talked to Mark and Emily about this, and we decided that OTX 2.0 will also not support python 3.9 for now. So I'm going to remove it from the CI test.

@harimkang
Copy link
Contributor

harimkang commented Feb 27, 2024

Can we skip python 3.9 checks for anomaly task? Anomalib v1 supports 3.10 and above

i create PR (#2991) - Merged

Copy link
Contributor

@harimkang harimkang left a comment

Choose a reason for hiding this comment

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

Can you please add integration tests to CI in the next PR? I think we need to add tasks to tox.ini and .github/workflows/pre_merge.yaml.

@ashwinvaidya17 ashwinvaidya17 enabled auto-merge (squash) February 27, 2024 11:52
@ashwinvaidya17 ashwinvaidya17 merged commit 8ac9f73 into openvinotoolkit:v2 Feb 28, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants