🚀 Anomalib Pipelines #2005

ashwinvaidya17 · 2024-04-17T13:22:38Z

📝 Description

Alternate design for pipeline

✨ Changes

Select what type of change your PR is:

🐞 Bug fix (non-breaking change which fixes an issue)
🔨 Refactor (non-breaking change which refactors the code base)
🚀 New feature (non-breaking change which adds functionality)
💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
📚 Documentation update
🔒 Security update

✅ Checklist

Before you submit your pull request, please make sure you have completed the following steps:

📋 I have summarized my changes in the CHANGELOG and followed the guidelines for my type of change (skip for minor changes, documentation updates, and test enhancements).
📚 I have made the necessary updates to the documentation (if applicable).
🧪 I have written tests that support my changes and prove that my fix is effective or my feature works (if applicable).

For more information about code review checklists, see the Code Review Checklist.

Signed-off-by: Ashwin Vaidya <[email protected]>

ashwinvaidya17 · 2024-04-22T14:36:59Z

Patchcore does not work with SerialRunner as rich progress bar clashes with corset subsampling's progress bar.

Signed-off-by: Ashwin Vaidya <[email protected]>

…re/pipeline_v2

samet-akcay

took me some time, but managed to do my first round :)

It is looking good so far! I've got some minor comments initially. Will go for another round later

samet-akcay · 2024-04-18T13:18:45Z

src/anomalib/cli/pipelines.py

+    PIPELINE_REGISTRY: dict[str, Orchestrator] | None = {
+        "benchmark": Benchmark(),
+    }


Can this fit into a single line?

src/anomalib/data/__init__.py

samet-akcay · 2024-04-19T08:47:48Z

src/anomalib/pipelines/jobs/benchmark/benchmark.py

+from anomalib.pipelines.utils import (
+    dict_from_namespace,
+    hide_output,
+)


Single line?

samet-akcay · 2024-04-19T08:48:51Z

src/anomalib/pipelines/jobs/base.py

+
+    @staticmethod
+    @abstractmethod
+    def get_iterator(args: Namespace | None = None) -> Iterator:


can we find a more descriptive name? Does this only returns configs each time? If yes, wouldn't iterator be too generic?

samet-akcay · 2024-04-19T08:51:25Z

src/anomalib/pipelines/jobs/benchmark/benchmark.py

+        result.to_csv(file_path, index=False)
+        self.logger.info(f"Saved results to {file_path}")
+
+    def _print_tabular_results(self, gathered_result: pd.DataFrame) -> None:


Would this be used anywhere else? If so, maybe we could move this to a util function to keep this class cleaner?

src/anomalib/pipelines/components/actions/grid_search.py

samet-akcay · 2024-04-24T10:24:40Z

src/anomalib/pipelines/components/base/pipeline.py

+log_file = "runs/pipeline.log"
+Path(log_file).parent.mkdir(exist_ok=True, parents=True)
+logger_file_handler = logging.FileHandler(log_file)
+logging.getLogger().addHandler(logger_file_handler)
+logging.getLogger().setLevel(logging.DEBUG)
+warnings.filterwarnings("ignore")
+for logger_name in ["lightning.pytorch", "lightning.fabric", "torchmetrics", "os"]:
+    logging.getLogger(logger_name).handlers = [logger_file_handler]
+format_string = "%(asctime)s - %(name)s - %(levelname)s - %(message)s"
+logging.basicConfig(format=format_string, level=logging.DEBUG)


Would it be possible to wrap this in a function, like setup_logging or something similar. It looks a bit messy this way

tests/integration/pipelines/__init__.py

tests/integration/pipelines/test_benchmark.py

samet-akcay · 2024-04-24T10:35:49Z

tools/experimental/README.md

@@ -0,0 +1,3 @@
+# Anomalib Experimental
+
+These are experimental utilities that are under development. These might change frequently or might even be dropped.


can we add this in a warning section

Co-authored-by: Samet Akcay <[email protected]>

Signed-off-by: Ashwin Vaidya <[email protected]>

Co-authored-by: Samet Akcay <[email protected]>

Signed-off-by: Ashwin Vaidya <[email protected]>

blaz-r · 2024-05-03T20:16:19Z

I glanced over the design real quick, seems great 😄. I'll do a more thorough overview asap, mostly from the viewpoint of the tiled ensemble.

djdameln

Thanks, I think we're getting there. I like the new Generator design which returns the job instance. My biggest concern with the current design is that the argument parsing is spread across multiple classes, which makes it a bit hard to follow. This may make it a bit intimidating for users to implement their own custom pipeline. Do you think we could simplify this in some way?

Since we want to encourage users to implement their own custom pipelines, we need to make sure that the functionality of the different components is very clear. I think it would be good to add a bit more detail to the docstrings of the classes (Pipeline, Job, Generator, Runner) including some examples to make it easier for the users to follow.

In line with this, I think it would be good to also add a guide to our documentation explaining step by step how to implement a new pipeline.

djdameln · 2024-05-06T12:02:31Z

src/anomalib/pipelines/components/runners/parallel.py

+
+
+class ParallelRunner(Runner):
+    """Run the job in parallel using a process pool."""


This could be a bit more descriptive and maybe show some examples

djdameln · 2024-05-06T12:03:56Z

src/anomalib/pipelines/components/runners/parallel.py

+    """Pool execution error should be raised when one or more jobs fail in the pool."""
+
+
+class ParallelRunner(Runner):


Is there a way to tell the runner which devices to use? Or does it always distribute the jobs across all available GPUs?

The parallel runner just creates an execution pool and passes the task_id to the job's run method. The job is responsible for using this task_id to use the appropriate device.

djdameln · 2024-05-06T12:12:09Z

src/anomalib/pipelines/benchmark/generator.py

+    """Generate BenchmarkJob."""
+
+    def __init__(self, accelerator: str) -> None:
+        self.accelerator = accelerator


I'm not sure about the terminology here. We use this variable mainly to distinguish between cpu and gpu, but I'm not sure if cpu is technically considered to be an accelerator. Maybe device would be a more suitable name?

Originally this was called device. I think we discussed on changing this to accelerator to be inline with lightning's terminology. I have no preference here. So, I can rename this once we finalise the name.

In Lightning, accelerator seems to be also used for CPUs
https://lightning.ai/docs/pytorch/stable/extensions/accelerator.html
https://lightning.ai/docs/pytorch/stable/api/lightning.pytorch.accelerators.CPUAccelerator.html

djdameln · 2024-05-06T12:51:51Z

src/anomalib/cli/cli.py

@@ -288,7 +294,7 @@ def instantiate_classes(self) -> None:
            self.model = self._get(self.config_init, "model")
            self._configure_optimizers_method_to_model()
            self.instantiate_engine()
-        else:
+        elif self.config["subcommand"] != "pipeline":


I would prefer anomalib benchmark ... but if someone implements a custom pipeline then I feel they should be able to run it without making changes to the cli. In this case they might have to use anomalib pipeline cutom_pipeline?

src/anomalib/pipelines/benchmark/generator.py

Signed-off-by: Ashwin Vaidya <[email protected]>

samet-akcay · 2024-05-14T07:15:10Z

tools/experimental/README.md

Would it be an idea to use warnings.warn in the entrypoint scripts to inform the user whenever they use these features?

Signed-off-by: Ashwin Vaidya <[email protected]>

samet-akcay

Looking good to me. My only feedback is the lack of examples in the docstring and documentation. If you would like to add this in a follow-up PR, that is also fine.

blaz-r · 2024-05-14T09:20:42Z

I will check this thoroughly in a few hours and leave some feedback.

blaz-r · 2024-05-14T13:39:22Z

For some reason I keep getting rich.errors.LiveError: Only one live display may be active at once when trying to run the benchmark, although I'm only using Padim.

blaz-r · 2024-05-14T13:51:16Z

src/anomalib/pipelines/components/base/pipeline.py

+        for runner in runners:
+            try:
+                _args = args.get(runner.generator.job_class.name, None)
+                runner.run(_args)
+            except Exception:  # noqa: PERF203 catch all exception and allow try-catch in loop
+                logger.exception("An error occurred when running the runner.")
+                print(
+                    f"There were some errors when running [red]{runner.generator.job_class.name}[/red] with"
+                    f" [green]{runner.__class__.__name__}[/green]."
+                    f" Please check [magenta]{log_file}[/magenta] for more details.",
+                )


How would this work if I wanted to take results from runner and use them as the input for the next one.
I see that a single runner collects the results at the end. But in case of tiled ensemble, training can be parallel, and then I'd have just a single serial runner that assembles the data back together. With this design I am not entirely sure how that would work.

Then I guess this design falls short 🙂 I'll have a look again at your code to see what changes need to be made.

Yeah. I think that having a pipeline, at least for me, would mean that you chain some elements and result of one is input for the next one. I also see no problem of using pipelines recursively (not a very deep recursion). So let's says in case of the tiled ensemble it'd look something like:
Training[Parallel]->merging[serial]->postprocessing(implemented as sub-pipeline consisting of serialRunners each doing one step like threshold, norm ...).
Tiled ensemble stores the data inside a class, that handles storage at different indices. Maybe this pattern of "storage" class can also be used in pipelines, so each runner returns that and it can be customized for each specific usecase.

blaz-r · 2024-05-14T13:52:41Z

I checked the code and the design is quite nice. I have just one question that I commented on the relevant part.

ashwinvaidya17 · 2024-05-14T14:07:52Z

For some reason I keep getting rich.errors.LiveError: Only one live display may be active at once when trying to run the benchmark, although I'm only using Padim.

I'll have a look. Even patchcore breaks due to rich progress bar in kcenter-greedy. Maybe we shouldn't merge it till this issue is not solved

* 🚀 Anomalib Pipelines (#2005) * Add initial design Signed-off-by: Ashwin Vaidya <[email protected]> * Refactor + add to CLI Signed-off-by: Ashwin Vaidya <[email protected]> * Support grid search on class path Signed-off-by: Ashwin Vaidya <[email protected]> * redirect outputs Signed-off-by: Ashwin Vaidya <[email protected]> * design v2 Signed-off-by: Ashwin Vaidya <[email protected]> * remove commented code Signed-off-by: Ashwin Vaidya <[email protected]> * add dummy experiment Signed-off-by: Ashwin Vaidya <[email protected]> * add config Signed-off-by: Ashwin Vaidya <[email protected]> * Refactor Signed-off-by: Ashwin Vaidya <[email protected]> * Add tests Signed-off-by: Ashwin Vaidya <[email protected]> * Apply suggestions from code review Co-authored-by: Samet Akcay <[email protected]> * address pr comments Signed-off-by: Ashwin Vaidya <[email protected]> * Apply suggestions from code review Co-authored-by: Samet Akcay <[email protected]> * refactor Signed-off-by: Ashwin Vaidya <[email protected]> * Simplify argparse Signed-off-by: Ashwin Vaidya <[email protected]> * modify logger redirect Signed-off-by: Ashwin Vaidya <[email protected]> * update docstrings Signed-off-by: Ashwin Vaidya <[email protected]> --------- Signed-off-by: Ashwin Vaidya <[email protected]> Co-authored-by: Samet Akcay <[email protected]> * 🐞 Fix Rich Progress with Patchcore Training (#2062) Add safe track Signed-off-by: Ashwin Vaidya <[email protected]> * [Pipelines] 🔨 Intra-stage result passing (#2061) * Add initial design Signed-off-by: Ashwin Vaidya <[email protected]> * Refactor + add to CLI Signed-off-by: Ashwin Vaidya <[email protected]> * Support grid search on class path Signed-off-by: Ashwin Vaidya <[email protected]> * redirect outputs Signed-off-by: Ashwin Vaidya <[email protected]> * design v2 Signed-off-by: Ashwin Vaidya <[email protected]> * remove commented code Signed-off-by: Ashwin Vaidya <[email protected]> * add dummy experiment Signed-off-by: Ashwin Vaidya <[email protected]> * add config Signed-off-by: Ashwin Vaidya <[email protected]> * Refactor Signed-off-by: Ashwin Vaidya <[email protected]> * Add tests Signed-off-by: Ashwin Vaidya <[email protected]> * Apply suggestions from code review Co-authored-by: Samet Akcay <[email protected]> * address pr comments Signed-off-by: Ashwin Vaidya <[email protected]> * Apply suggestions from code review Co-authored-by: Samet Akcay <[email protected]> * refactor Signed-off-by: Ashwin Vaidya <[email protected]> * Simplify argparse Signed-off-by: Ashwin Vaidya <[email protected]> * modify logger redirect Signed-off-by: Ashwin Vaidya <[email protected]> * update docstrings Signed-off-by: Ashwin Vaidya <[email protected]> * Add proposal Signed-off-by: Ashwin Vaidya <[email protected]> --------- Signed-off-by: Ashwin Vaidya <[email protected]> Co-authored-by: Samet Akcay <[email protected]> * Update src/anomalib/pipelines/benchmark/job.py --------- Signed-off-by: Ashwin Vaidya <[email protected]> Co-authored-by: Samet Akcay <[email protected]>

ashwinvaidya17 added 9 commits April 4, 2024 16:38

Add initial design

a4778d7

Signed-off-by: Ashwin Vaidya <[email protected]>

Refactor + add to CLI

29af8fd

Signed-off-by: Ashwin Vaidya <[email protected]>

Support grid search on class path

246ca2b

Signed-off-by: Ashwin Vaidya <[email protected]>

redirect outputs

051566a

Signed-off-by: Ashwin Vaidya <[email protected]>

design v2

0641e16

Signed-off-by: Ashwin Vaidya <[email protected]>

remove commented code

a9cb590

Signed-off-by: Ashwin Vaidya <[email protected]>

add dummy experiment

b4b981b

Signed-off-by: Ashwin Vaidya <[email protected]>

add config

eaca521

Signed-off-by: Ashwin Vaidya <[email protected]>

Refactor

d279e75

Signed-off-by: Ashwin Vaidya <[email protected]>

ashwinvaidya17 added 2 commits April 23, 2024 16:06

Add tests

979934c

Signed-off-by: Ashwin Vaidya <[email protected]>

Merge branch 'main' of github.com:openvinotoolkit/anomalib into featu…

f45a0fd

…re/pipeline_v2

ashwinvaidya17 marked this pull request as ready for review April 23, 2024 14:07

ashwinvaidya17 requested review from samet-akcay and djdameln as code owners April 23, 2024 14:07

samet-akcay changed the title ~~🚀 Initial pipeline design v2~~ 🚀 Anomalib Pipelines Apr 23, 2024

samet-akcay requested changes Apr 24, 2024

View reviewed changes

Apply suggestions from code review

6efaf42

Co-authored-by: Samet Akcay <[email protected]>

samet-akcay mentioned this pull request Apr 29, 2024

Question about ensemble tiler tool #2036

Closed

ashwinvaidya17 and others added 3 commits May 1, 2024 13:39

address pr comments

b4db761

Signed-off-by: Ashwin Vaidya <[email protected]>

Apply suggestions from code review

59a480d

Co-authored-by: Samet Akcay <[email protected]>

refactor

c505236

Signed-off-by: Ashwin Vaidya <[email protected]>

djdameln reviewed May 6, 2024

View reviewed changes

ashwinvaidya17 added 2 commits May 10, 2024 10:16

Simplify argparse

8d64be8

Signed-off-by: Ashwin Vaidya <[email protected]>

modify logger redirect

997fb40

Signed-off-by: Ashwin Vaidya <[email protected]>

ashwinvaidya17 requested review from samet-akcay and djdameln May 14, 2024 06:54

samet-akcay reviewed May 14, 2024

View reviewed changes

update docstrings

b7ef7dc

Signed-off-by: Ashwin Vaidya <[email protected]>

samet-akcay approved these changes May 14, 2024

View reviewed changes

blaz-r reviewed May 14, 2024

View reviewed changes

ashwinvaidya17 changed the base branch from main to feature/pipelines May 15, 2024 09:11

ashwinvaidya17 merged commit 5ff7f10 into openvinotoolkit:feature/pipelines May 15, 2024
5 of 7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🚀 Anomalib Pipelines #2005

🚀 Anomalib Pipelines #2005

ashwinvaidya17 commented Apr 17, 2024

ashwinvaidya17 commented Apr 22, 2024

samet-akcay left a comment

samet-akcay Apr 18, 2024

samet-akcay Apr 19, 2024

samet-akcay Apr 19, 2024

samet-akcay Apr 19, 2024

samet-akcay Apr 24, 2024

samet-akcay Apr 24, 2024

blaz-r commented May 3, 2024

djdameln left a comment

djdameln May 6, 2024

djdameln May 6, 2024

ashwinvaidya17 May 7, 2024

djdameln May 6, 2024

ashwinvaidya17 May 7, 2024

samet-akcay May 14, 2024

djdameln May 6, 2024

samet-akcay May 14, 2024

samet-akcay left a comment

blaz-r commented May 14, 2024

blaz-r commented May 14, 2024

blaz-r May 14, 2024 •

edited

Loading

ashwinvaidya17 May 14, 2024

blaz-r May 14, 2024 •

edited

Loading

blaz-r commented May 14, 2024

ashwinvaidya17 commented May 14, 2024

		@@ -0,0 +1,3 @@
		# Anomalib Experimental

		These are experimental utilities that are under development. These might change frequently or might even be dropped.



		class ParallelRunner(Runner):
		"""Run the job in parallel using a process pool."""

		"""Pool execution error should be raised when one or more jobs fail in the pool."""


		class ParallelRunner(Runner):

🚀 Anomalib Pipelines #2005

🚀 Anomalib Pipelines #2005

Conversation

ashwinvaidya17 commented Apr 17, 2024

📝 Description

✨ Changes

✅ Checklist

ashwinvaidya17 commented Apr 22, 2024

samet-akcay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blaz-r commented May 3, 2024

djdameln left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samet-akcay left a comment

Choose a reason for hiding this comment

blaz-r commented May 14, 2024

blaz-r commented May 14, 2024

blaz-r May 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blaz-r May 14, 2024 • edited Loading

Choose a reason for hiding this comment

blaz-r commented May 14, 2024

ashwinvaidya17 commented May 14, 2024

blaz-r May 14, 2024 •

edited

Loading

blaz-r May 14, 2024 •

edited

Loading