Initial commit

[vinnamkim]

Summary

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

[wonjuleee]

LGTM

[sungmanc]

What do you think about the dir structure below? (Gathering entities)
I believe users need to change more for model/entity than model/module when they want to bring new models. Categorizing the entity itself looks better to me. I know it could be different from person to person. So, it's just a suggestion.

root/
    algo/                       # Custom algo (e.g., hierarchical_cls_head)
    cli/                        # CLI entrypoints
    config/                     # Default YAML config files
    core/
        config/                 # Structured data type object for configurations
        entities/ ## OTX entities
            model/
                base.py
                detection.py
                ...
            data/
                base.py
                detection.py
                ...
        data/                   # Data related things
            dataset/            # OTXDataset
                base.py
                detection.py
                ...
            transform_libs/     # To support transform libraries (e.g., MMCV)
            factory.py          # Factory to instantiate data related objects
            module.py           # OTXDataModule
        engine/                 # PyTorchLightning engine
            train.py
            ...
        model/                  # Model related things
            module/             # OTXLitModule
                base.py
                detection.py
                ...
        types/                  # Enum definitions (e.g. OTXTaskType)
        utils/                  # Utility functions
    recipe/                     # Recipe YAML config for each model we support
        detection/              # (e.g., rtmdet_tiny)
        ...
    tools/                      # Python runnable scripts for some TBD use cases

[kprokofi]

Overall, new design looks good to me. On the next refactoring stage I propose to move "recipe" to configs folder, because it is confusing that we have configs in different places + I would rename it. "Recipe" is not appropriate name for model configs.
Also, we should think about folder structure under "algo" folder.
Next, I personally don't understand why we use Structured Configs not for all Yaml config groups from "src/configs". And overall, for me, it is a bit complicated structure using these Structured Configs since different parts of configs located in different places and use two different types of configs (yaml and python dataclasses).
Last thing, creating types like "Transforms = Union[Callable, List[Callable]]", "class OTXBatchLossEntity(Dict[str, Tensor])", etc. seems to be unnecessarily overhead
And of course, we should make Engine to be a Python class

Thank you for a such hard work!

[vinnamkim]

Overall, new design looks good to me. On the next refactoring stage I propose to move "recipe" to configs folder, because it is confusing that we have configs in different places + I would rename it. "Recipe" is not appropriate name for model configs.

Recipe is not just a model config. Recipe is a literally "recipe" which includes everything to make a deep learning model. It includes not just model configs, but also data transforms, optimizer, LR scheduler, callbacks, .... I did many comments about this to @eugene123tw, but saying it again here. In our design, the recipe is who overrides the base configs (src/otx/config/**) at the very last moment. That's why we should split them in the separate place (but other small reasons exists, e.g., complying Hydra's rules, as well).

And, I don't think that "recipe" seems not weird literally in the concept I've been mentioned. I just googled about it and can found an article using the word in this way:

http://karpathy.github.io/2019/04/25/recipe/
https://wandb.ai/craiyon/report/reports/A-Recipe-for-Training-Large-Models--VmlldzozNjc4MzQz

+You guys have also https://github.com/openvinotoolkit/training_extensions/tree/develop/src/otx/recipes/stages

However, if this is really weird to you guys opinion. Yes, we can rename it, but it cannot be mold into src/otx/config.

Also, we should think about folder structure under "algo" folder. Next, I personally don't understand why we use Structured Configs not for all Yaml config groups from "src/configs". And overall, for me, it is a bit complicated structure using these Structured Configs since different parts of configs located in different places and use two different types of configs (yaml and python dataclasses).

I've been talking about this many time but again. Structure config is important. Let's see this

training_extensions/src/otx/v2/adapters/torch/mmengine/modules/utils/config_utils.py

Lines 35 to 278 in b47eaae

	class CustomConfig(Config):
	"""A class that extends the base `Config` class, adds additional functionality for loading configuration files."""
	@staticmethod
	def _file2dict(
	filename: str,
	use_predefined_variables: bool = True,
	) -> Tuple[Config, str]:
	"""Static method that loads the configuration file and returns a dictionary of its contents.
	:param filename: str, the path of the configuration file to be loaded.
	:param use_predefined_variables: bool, a flag indicating whether to substitute predefined variables in the
	configuration file.
	:return: tuple of dictionary and string. Returns a dictionary containing the contents of the configuration file
	and a string representation of the configuration file.
	:raises: IOError if the file type is not supported.
	"""
	filename = osp.abspath(osp.expanduser(filename))
	check_file_exist(filename)
	extender = osp.splitext(filename)[1]
	if extender not in [".py", ".json", ".yaml", ".yml"]:
	raise OSError("Only py/yml/yaml/json type are supported now!")
	with tempfile.TemporaryDirectory() as temp_config_dir:
	with tempfile.NamedTemporaryFile(dir=temp_config_dir, suffix=extender) as temp_config_file:
	if platform.system() == "Windows":
	temp_config_file.close()
	temp_config_name = osp.basename(temp_config_file.name)
	# Substitute predefined variables
	if use_predefined_variables:
	Config._substitute_predefined_vars(filename, temp_config_file.name)
	else:
	shutil.copyfile(filename, temp_config_file.name)
	# Substitute base variables from placeholders to strings
	base_var_dict = Config._pre_substitute_base_vars(temp_config_file.name, temp_config_file.name)
	if filename.endswith(".py"):
	temp_module_name = osp.splitext(temp_config_name)[0]
	sys.path.insert(0, temp_config_dir)
	Config._validate_py_syntax(filename)
	mod = import_module(temp_module_name)
	sys.path.pop(0)
	cfg_dict = {name: value for name, value in mod.__dict__.items() if not name.startswith("__")}
	# delete imported module
	del sys.modules[temp_module_name]
	elif filename.endswith((".yml", ".yaml", ".json")):
	import mmengine
	cfg_dict = mmengine.load(temp_config_file.name)
	# check deprecation information
	if DEPRECATION_KEY in cfg_dict:
	deprecation_info = cfg_dict.pop(DEPRECATION_KEY)
	warning_msg = f"The config file {filename} will be deprecated " "in the future."
	if "expected" in deprecation_info:
	warning_msg += f' Please use {deprecation_info["expected"]} ' "instead."
	if "reference" in deprecation_info:
	warning_msg += " More information can be found at " f'{deprecation_info["reference"]}'
	warnings.warn(warning_msg)
	cfg_text = filename + "\n"
	with open(filename, encoding="utf-8") as f:
	# Setting encoding explicitly to resolve coding issue on windows
	cfg_text += f.read()
	if BASE_KEY in cfg_dict:
	cfg_dir = osp.dirname(filename)
	base_key = cfg_dict.pop(BASE_KEY)
	base_filename: List[str] = base_key if isinstance(base_key, list) else [base_key]
	cfg_dict_list = []
	cfg_text_list = []
	for file_path in base_filename:
	_cfg_dict, _cfg_text = CustomConfig._file2dict(osp.join(cfg_dir, file_path))
	cfg_dict_list.append(_cfg_dict)
	cfg_text_list.append(_cfg_text)
	base_cfg_dict: Union[Config, Dict] = {}
	# for c in cfg_dict_list:
	# if len(duplicate_keys) > 0:
	# raise KeyError('Duplicate key is not allowed among bases. '
	# f'Duplicate keys: {duplicate_keys}')
	for c in cfg_dict_list:
	if len(base_cfg_dict.keys() & c.keys()) > 0:
	logger.warning(f"Duplicate key is detected among bases [{base_cfg_dict.keys() & c.keys()}]")
	logger.debug(f"base = {base_cfg_dict}, cfg = {c}")
	base_cfg_dict = Config._merge_a_into_b(base_cfg_dict, c)
	logger.debug(f"merged dict = {base_cfg_dict}")
	else:
	base_cfg_dict.update(c)
	# Subtitute base variables from strings to their actual values
	cfg_dict = Config._substitute_base_vars(cfg_dict, base_var_dict, base_cfg_dict)
	base_cfg_dict = Config._merge_a_into_b(cfg_dict, base_cfg_dict)
	cfg_dict = base_cfg_dict
	# merge cfg_text
	cfg_text_list.append(cfg_text)
	cfg_text = "\n".join(cfg_text_list)
	return cfg_dict, cfg_text
	@staticmethod
	def fromfile(filename: str, use_predefined_variables: bool = True, import_custom_modules: bool = True) -> Config:
	"""Static method that loads a configuration file and returns an instance of `Config` class.
	:param filename: str, the path of the configuration file to be loaded.
	:param use_predefined_variables: bool, a flag indicating whether to substitute predefined variables in the
	configuration file.
	:param import_custom_modules: bool, a flag indicating whether to import custom modules.
	:return: Config object, an instance of `Config` class containing the contents of the configuration file.
	"""
	cfg_dict, cfg_text = CustomConfig._file2dict(filename, use_predefined_variables)
	if import_custom_modules and cfg_dict.get("custom_imports", None):
	import_modules_from_strings(**cfg_dict["custom_imports"])
	return CustomConfig(cfg_dict, cfg_text=cfg_text, filename=filename)
	@staticmethod
	def to_dict(config: Config | ConfigDict) -> dict:
	"""Converts a Config object to a dictionary.
	Args:
	config(Config): The Config object to convert.
	Return:
	dict: The resulting dictionary.
	"""
	output_dict = {}
	for key, value in config.items():
	if isinstance(value, (Config, ConfigDict)):
	output_dict[key] = CustomConfig.to_dict(value)
	else:
	output_dict[key] = value
	return output_dict
	@property
	def pretty_text(self) -> str:
	"""Make python file human-readable.
	It's almost same as mmengine.Config's code but code to reformat using yapf is removed to reduce time.
	"""
	indent = 4
	def _indent(s_: str, num_spaces: int) -> str:
	s = s_.split("\n")
	if len(s) == 1:
	return s_
	first = s.pop(0)
	s = [(num_spaces * " ") + line for line in s]
	_s = "\n".join(s)
	_s = first + "\n" + _s
	return _s
	def _format_basic_types(k: str, v: list, use_mapping: bool = False) -> str:
	v_str = f"'{v}'" if isinstance(v, str) else str(v)
	if use_mapping:
	k_str = f"'{k}'" if isinstance(k, str) else str(k)
	attr_str = f"{k_str}: {v_str}"
	else:
	attr_str = f"{str(k)}={v_str}"
	attr_str = _indent(attr_str, indent)
	return attr_str
	def _format_list(k: str, v: list, use_mapping: bool = False) -> str:
	# check if all items in the list are dict
	if all(isinstance(_, dict) for _ in v):
	v_str = "[\n"
	v_str += "\n".join(f"dict({_indent(_format_dict(v_), indent)})," for v_ in v).rstrip(",")
	if use_mapping:
	k_str = f"'{k}'" if isinstance(k, str) else str(k)
	attr_str = f"{k_str}: {v_str}"
	else:
	attr_str = f"{str(k)}={v_str}"
	attr_str = _indent(attr_str, indent) + "]"
	else:
	attr_str = _format_basic_types(k, v, use_mapping)
	return attr_str
	def _contain_invalid_identifier(dict_str: dict) -> bool:
	contain_invalid_identifier = False
	for key_name in dict_str:
	contain_invalid_identifier |= not str(key_name).isidentifier()
	return contain_invalid_identifier
	def _format_dict(input_dict: dict, outest_level: bool = False) -> str:
	r = ""
	s = []
	use_mapping = _contain_invalid_identifier(input_dict)
	if use_mapping:
	r += "{"
	for idx, (k, v) in enumerate(input_dict.items()):
	is_last = idx >= len(input_dict) - 1
	end = "" if outest_level or is_last else ","
	if isinstance(v, dict):
	v_str = "\n" + _format_dict(v)
	if use_mapping:
	k_str = f"'{k}'" if isinstance(k, str) else str(k)
	attr_str = f"{k_str}: dict({v_str}"
	else:
	attr_str = f"{str(k)}=dict({v_str}"
	attr_str = _indent(attr_str, indent) + ")" + end
	elif isinstance(v, list):
	attr_str = _format_list(k, v, use_mapping) + end
	else:
	attr_str = _format_basic_types(k, v, use_mapping) + end
	s.append(attr_str)
	r += "\n".join(s)
	if use_mapping:
	r += "}"
	return r
	cfg_dict = self._cfg_dict.to_dict()
	text = _format_dict(cfg_dict, outest_level=True)
	return text
	@staticmethod
	def merge_cfg_dict(base_dict: Union[Config, Dict], cfg_dict: Union[Config, Dict]) -> dict:
	if isinstance(base_dict, Config):
	base_dict = base_dict._cfg_dict.to_dict()
	if isinstance(cfg_dict, Config):
	cfg_dict = cfg_dict._cfg_dict.to_dict()
	return CustomConfig._merge_a_into_b(cfg_dict, base_dict)
	def dump(self, file: Optional[Union[str, Path]] = None) -> None:
	"""Dump config to file or return config text.
	Args:
	file (str or Path, optional): If not specified, then the object
	is dumped to a str, otherwise to a file specified by the filename.
	Defaults to None.
	Returns:
	str or None: Config text.
	"""
	# if file is None:
	# if self.filename is None or self.filename.endswith('.py'):
	# with open(file, 'w', encoding='utf-8') as f:

You never catch which variables Config has from the code. You can get it only at the runtime or from other sources (e.g., your prior knowledge). If you are a new comer and should develop something using Config such as the following code.

training_extensions/src/otx/v2/adapters/torch/mmengine/mmaction/engine.py

Lines 47 to 60 in b47eaae

	def _update_config(self, func_args: dict, **kwargs) -> tuple[Config, bool]:
	config, update_check = super()._update_config(func_args, **kwargs)
	for subset in ("val", "test"):
	if f"{subset}_dataloader" in config and config[f"{subset}_dataloader"] is not None:
	evaluator_config = self._get_value_from_config(f"{subset}_evaluator", func_args)
	config[f"{subset}_evaluator"] = self._update_eval_config(evaluator_config=evaluator_config)
	if hasattr(config, "visualizer") and config.visualizer.type not in VISUALIZERS:
	config.visualizer = {
	"type": "ActionVisualizer",
	"vis_backends": [{"type": "LocalVisBackend"}, {"type": "TensorboardVisBackend"}],
	}
	return config, update_check

Let's imagine that you are in the middle of implementing this from scratch. Do you think that it is possible to catch config can have visualizer variable without any prior knowledge?

On the other hand, let's see this code

training_extensions/src/otx/core/data/module.py

Lines 30 to 41 in 61c09ab

	def __init__(self, task: OTXTaskType, config: DataModuleConfig) -> None:
	"""Constructor."""
	super().__init__()
	self.task = task
	self.config = config
	self.subsets: dict[str, OTXDataset] = {}
	self.save_hyperparameters()
	dataset = DmDataset.import_from(
	self.config.data_root,
	format=self.config.data_format,
	)

I think that you have an IDE. If your IDE is visual studio code. You can just move your cursor to DataModuleConfig, click the F12 key in your keyboard, then you can get this information. This is a piece of cake.

training_extensions/src/otx/core/config/data.py

Lines 24 to 28 in 61c09ab

	class DataModuleConfig:
	"""DTO for data module configuration."""
	data_format: str
	data_root: str
	subsets: dict[str, SubsetConfig]

Last thing, creating types like "Transforms = Union[Callable, List[Callable]]", "class OTXBatchLossEntity(Dict[str, Tensor])", etc. seems to be unnecessarily overhead And of course, we should make Engine to be a Python class

This makes such a small learning curve at the first time, but I believe that the hard typing is better for development cycle then the soft typing. It is same as you guys not feeling any cumbersome for mm* since you guys have been training on them such a long time and have piled many experiences about it. If the hard typing is not useful, why did people invented these things?

1. https://mypy.readthedocs.io/en/stable/ (even this is used for your project)
2. https://www.typescriptlang.org/

[sungmanc]

Still we need to decide related to the pre-commit, for my side, I'm okay to merge AS-IS.

[harimkang]

@eugene123tw , @kprokofi , @sungmanc. @jaegukhyun , @vinnamkim
I think I can enable ruff's "UP007" rule, I'll create a PR for this.
As for the rest, I think most of the rules related to docstrings are ignored, but we'll have to discuss this further. Let's come up with a docstring convention and fix it together.

[samet-akcay]

This could also be just models

[vinnamkim]

It's not just for model see #2682

[samet-akcay]

I disagree to this. We should use UP007, since it is the new suggested format.

[samet-akcay]

Unless we have a really good reason, and agreed by everyone, we should not change the rules here. These are to imrove the overall code quality.

[samet-akcay]

This approach is a common approach in mmx ecosystem. However, as a developer, I find this hard to follow. In every other config file that refers to this default.yaml file, we need to go back to this file. Due to this limitation, new mmx api released a new mechanism just to properly navigate between config files.

[samet-akcay]

what is the real advantage of this DTO again? This seems that all these parameters are manually set here. Would it be an idea to consider a more dynamic approach here?

[vinnamkim]

I'd like to ask you what is the real disadvantage of DTO? It is just a group of arguments. People feels uncomfortable if there are so many arguments not organized. I believe it is not just my personal feeling, https://docs.astral.sh/ruff/rules/too-many-arguments/.

Let's see this example.

from dataclasses import dataclass
from jsonargparse import CLI

@dataclass
class TaskInfo:
    task_type: str | None = "good_task"

@dataclass
class MyConfig:
    """My engine config

    Args:
        task_type: My task type
        data_root: Root path for dataset
        learning_rate: Learning rate for model optimizer
        test_after_training: If true, execute the test pipeline for the best validation fit model checkpoint
    """
    task_info: TaskInfo
    data_root: str
    learning_rate: float
    test_after_training: bool = True

class MyDataModule:
    def __init__(self, data_root: str):
        pass

class Engine:
    """My Engine class

    Args:
        engine_cfg: configuration for my engine
    """

    def __init__(
        self,
        task_info: TaskInfo,
        data_root: str,
        learning_rate: float,
        test_after_training: bool,
    ):
        self.data_module = MyDataModule(data_root=data_root)
        pass

    @classmethod
    def from_config(cls, engine_cfg: MyConfig) -> "Engine":
        return cls(
            task_type=engine_cfg.task_info,
            data_root=engine_cfg.data_root,
            learning_rate=engine_cfg.learning_rate,
            test_after_training=engine_cfg.test_after_training,
        )

if __name__ == "__main__":
    CLI(Engine.from_config)

(otx-v2) vinnamki@vinnamki:~/otx/training_extensions$ python test.py --help
usage: test.py [-h] [--config CONFIG] [--print_config[=flags]] [--engine_cfg CONFIG] [--engine_cfg.task_info CONFIG] [--engine_cfg.task_info.task_type TASK_TYPE] --engine_cfg.data_root DATA_ROOT
               --engine_cfg.learning_rate LEARNING_RATE [--engine_cfg.test_after_training {true,false}]

<bound method Engine.from_config of <class '__main__.Engine'>>

options:
  -h, --help            Show this help message and exit.
  --config CONFIG       Path to a configuration file.
  --print_config[=flags]
                        Print the configuration after applying all other arguments and exit. The optional flags customizes the output and are one or more keywords separated by comma. The supported flags
                        are: comments, skip_default, skip_null.

My engine config:
  --engine_cfg CONFIG   Path to a configuration file.
  --engine_cfg.data_root DATA_ROOT
                        Root path for dataset (required, type: str)
  --engine_cfg.learning_rate LEARNING_RATE
                        Learning rate for model optimizer (required, type: float)
  --engine_cfg.test_after_training {true,false}
                        If true, execute the test pipeline for the best validation fit model checkpoint (type: bool, default: True)

TaskInfo(task_type: str | None = 'good_task'):
  --engine_cfg.task_info CONFIG
                        Path to a configuration file.
  --engine_cfg.task_info.task_type TASK_TYPE
                        (type: str | None, default: good_task)

It just can add one degree of freedom to create Engine from the DTO, not only from the very long explicit arguments. I cannot understand why you think they are mutually exclusive each other.

In addition, there is clear advantage during the development phase. You see that Engine has a member object MyDataModule. data_root should be propagated from Engine to MyDataModule. As you can see, we have to fix more than two places for refactoring or newly added variables. It makes our early development phase slow.

[samet-akcay]

Lightning Trainer has more args defined. Why we only have these here? Also, the parameters in Lightning Trainer frequently change, which might cause some issues here that we need to maintain. Again, I'd prefer a more dynamic approach

[vinnamkim]

I believe that it is our mission to provide complete training templates (the recipe named in this PR) for Intel device (I guess you read Yannis's reply Mark forwarded to you). Those templates should be reproducible and can enable our users to estimate their cost to solve their problem with our tools.

https://github.com/open-mmlab/mmdetection/blob/main/configs/rtmdet/README.md

It is a very good example for this from mmx. I personally don't like their code structures and the learning curve to learn mm things is steep. However, you know, this scene has so much reproduction problem and the computation cost is not free (even very costly for deep learning workload, e.g. GPUs). I think that the cost to learn mmx is smaller than the computation cost wasted for trials and errors. I think that everyone who acts economically thinks so as well.

At this time, the complete training templates can have the different parameters from the defaults of PyTorch Lightning (especially for Intel devices since the major player is NVIDIA in this scene).

[samet-akcay]

what does dm mean?

[samet-akcay]

ah found that it refers to datumaro. Here is my next question, why do we need to refer to datumaro subset?

[samet-akcay]

so, let's assume that I don't have a datumaro background, but would like to use OTX. How can I do that?

[samet-akcay]

Also let's assume that I'm using the API, and would like to create OTXDataset. In this case I will have to create Subset stuff first, right?

If yes, is there any easier way to create the object with a single liner?

[vinnamkim]

You can see https://github.com/vinnamkim/training_extensions/blob/6128725869888f45b323f99ab8caaa9d2ab302a9/src/otx/core/data/module.py#L30

This is the entrypoint I'm planning for Python API.

[samet-akcay]

How about subset?

Suggested change

	def __init__(self, dm_subset: DatasetSubset, transforms: Transforms) -> None:
	def __init__(self, subset: DatasetSubset, transforms: Transforms) -> None:

[vinnamkim]

There are so many renaming comments here. Please see my other comments for other naming things too.

[samet-akcay]

Can you elaborate why this is needed?

[samet-akcay]

Is this the new OTXDatasetItem from v1?

[samet-akcay]

I feel we might need a more descriptive name for this dataclass. Data entity is a bit a broad term. Looking at the attributes of this class, i would say this could be called something like OtxDatasetItem, OtxImage etc.

[vinnamkim]

Please see my other comments.

[samet-akcay]

Does this only store the shape info? Or any plans to add some other metadata as well? depending on this, this might be renamed to some alternatives for clarity maybe. ImageMetadata, ImageShapeInfo, ImageShapeMetadata?

[samet-akcay]

Is this needed for thee development only? Will it be kept after the release

[vinnamkim]

We are hardly under development now. Absolutely, it's temporal.

[samet-akcay]

can you clarify the purpose of this default yaml file? Not clear why this is needed?

[vinnamkim]

Please see src/otx/config/train.yaml. It refers this file.

[samet-akcay]

I'm not sure if I like this approach. I think this is one of the main limitations of mmx ecosystem. When I look at a mmx config file, I found extremely annoying to navigate around to find the base config stuff. I understand that this is to avoid duplication; however, there should be trade-off between duplication and abstraction.

This will make the config navigation so difficult that when the reader needs to see a parameter, they will need to navigate around to see what is inside this default stuff.

In fact, even mmx developers agreed to this and introduced a new feature to make this a bit easier.

[vinnamkim]

If overriding by groups causes more loss than gain, then of course we will give up. However, we don't know for now how much the gain will be since this project is at very early stage. That decision can be made in later.

[samet-akcay]

are these files automatically or manually generated?

[samet-akcay]

if the latter, is a developer supposed to update these files when there is a change in these configurations parameters?

[vinnamkim]

Yes, if we want to keep our own default arguments for this, it is not evitable you know. I guess you might say about it like "No, JSONARGPARSE can do ...". I don't think so. For example, if lightning.pytorch.callbacks.EarlyStopping has default arguments in its construction, it can do the same such as

early_stopping:
  _target_: lightning.pytorch.callbacks.EarlyStopping

It will initiate lightning.pytorch.callbacks.EarlyStopping() with its default arguments.

[samet-akcay]

This is a future reference for everyone. Please limit the use of abbreviations within the code base. Abbvreviations might be clear to the writer, but not the reader. It hurts the readability.

[samet-akcay]

What does transform_lib_type mean? You mean like these transforms are coming from the mmdetection library?

[vinnamkim]

You mean like these transforms are coming from the mmdetection library?

Yes

[samet-akcay]

what is the motivation of having debug configs?

[vinnamkim]

It enables us to turn on debug related configurations easily.

[samet-akcay]

Can you clarify why this file is needed, and why it is empty?

[vinnamkim]

Thanks, we can remove it.

[samet-akcay]

Are we going to have more parameters in the future? If no, not sure if this is a good idea to store a separate config for 2 lines

[vinnamkim]

Saying again. We are very early stage and we cannot estimate how much this file will be bloating in the future currently. I'll absolutely restructure this later if needed. As you mentioned, refactoring should be done continuously.

[samet-akcay]

Instead of referring to the default config that has only two lines, can't we just add those parameters here?

[vinnamkim]

Please see my other comments.

[samet-akcay]

again, as a user/developer, I usually prefer to see the parameters explicitly here. With this approach, I'll need to refer to the default config file, which causes coupling

[vinnamkim]

Please see my other comments.

[samet-akcay]

Not sure TransformLibType is descriptive enough. Would it be possible to have a more descriptive name to reveal what it actually is?

[vinnamkim]

Please see my other comments.

[samet-akcay]

I'm still trying to understand the real value of this DTO. With this approach, we need to create one yaml file and one python file for each of these, no?

Can you elaborate the advantages of this approach?

[vinnamkim]

Developers can look at how the configuration variables are propagated in the code in a structured way.

[samet-akcay]

This is following the old annotation format. Can you use the new annotation suggested by pep

[vinnamkim]

Sorry it's my fault. @harimkang is preparing a fix for this.

[samet-akcay]

Also let's assume that I'm using the API, and would like to create OTXDataset. In this case I will have to create Subset stuff first, right?

If yes, is there any easier way to create the object with a single liner?

[samet-akcay]

what would be the difference in OTXBatchLossEntity when we have a single image or a batch? I mean do we need a batch loss entity? Wouldn't having an OtxLoss do the job?

[vinnamkim]

I wanted to make naming consistency between T_OTXBatchPredEntity and OTXBatchLossEntity

[samet-akcay]

so if the above suggestions worked, this would be simplified to something like the following?

def forward(self, inputs: OtxBatch) -> OtxBatchPrediction | OtxBathLoss

[samet-akcay]

or if there is not much difference between OtxBatchLoss and OtxLoss it could also be just OtxLoss

[vinnamkim]

Please see my above comments.

[samet-akcay]

again, as I mentioned above, config dependency will really hurt the API use.

[vinnamkim]

In this case it is inevitable to build mmx models. It's the way how mmx gonna work.

[samet-akcay]

Since the name of the method is _customize_inputs, would it be an idea to rename entity to inputs?

[vinnamkim]

Please see my above comments.

[samet-akcay]

the previous OTXModel is also a module, right? It's inheriting nn.Module. Can you clarify why it is in a different module? It seems to me that the only difference is that the former is a TorchModel, while the latter is a LightningModel, no?

[vinnamkim]

Model architectures can be different. But, they share the similar loss and metric computations task-wise. That's why I decided to inject OTXModel to OTXLitModule.