-
Notifications
You must be signed in to change notification settings - Fork 443
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
[RFC] Engine Refactor Proposal | Alternative 2 #3760
base: develop
Are you sure you want to change the base?
[RFC] Engine Refactor Proposal | Alternative 2 #3760
Conversation
Signed-off-by: Ashwin Vaidya <ashwinnitinvaidya@gmail.com>
"""Mock autoconfigurator for the engine.""" | ||
|
||
def __init__(self, model: nn.Module | None = None, data_root: Path | None = None, task: str | None = None): | ||
self._engine = self._configure_engine(model) # ideally we want to pass the data_root and task as well |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, the role of auto-configuration is to check for task, data, and model inputs, regardless of the engine, and provide default settings for anything the user hasn't entered. Is there any reason to configure the engine internally? If it's just for the backend, it would be nice to have a different way to configure the default for each backend rather than configuring the engine directly. What do you think?
**kwargs, | ||
) -> BaseEngine: | ||
"""This takes in all the parameters that are currently passed to the OTX Engine's `__init__` method.""" | ||
autoconfigurator = AutoConfigurator(model, data_root=data_root, **kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Engine -> AutoConfigurator -> Engine : I think their relationship with each other is strange.
ANNOTATIONS = Any | ||
|
||
|
||
class BaseEngine(ABC): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we talked about, the arguments in BaseEngine
will be the same as those in the current otx Engine, only the Type will be made more general as needed, right?
pass | ||
|
||
@abstractmethod | ||
def train(self, model: nn.Module, **kwargs) -> METRICS: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's still a model here, which I think might be confusing for people looking at this PR.
Motivation
Refer to #3752 for the motivation
This PR proposes an alternative design. It also includes one solution to register heterogeneous models to the CLI (there might be better approaches).