Skip to content

MP-002: Add ml dependency injection#3

Open
jayyaali95 wants to merge 1 commit intomainfrom
jayyaali/mp-002
Open

MP-002: Add ml dependency injection#3
jayyaali95 wants to merge 1 commit intomainfrom
jayyaali/mp-002

Conversation

@jayyaali95
Copy link
Copy Markdown
Owner

No description provided.

@jayyaali95 jayyaali95 requested a review from viktorb-lyft May 9, 2025 23:46
pass

@classmethod
def create(cls, config: Union[str, Dict[str, Any]]):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Seems a bit of duplication between other base classes. This could be exported into a factory class

self.model.set_params(params)


class Adam(Optimizer):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice!

return self.transform(data)

@classmethod
def create(cls, config: Union[str, Dict[str, Any]]):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same as above, could be just Factory.create()


base_classes = [
DataLoader, MetricFunction, Tracker, Preprocessor,
TrainLoop, Optimizer, Model
Copy link
Copy Markdown
Collaborator

@viktorb-lyft viktorb-lyft May 16, 2025

Choose a reason for hiding this comment

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

Instead of creating this list here, can we use Factory. register_component_type to add it to this list? Also this decorator could register the keyword in yaml (e.g. 'preprocessor' that you can leverage in create().

(hasattr(param_type, "__origin__") and
param_type.__origin__ is Optional and
param_type.__args__[0] == bc)):
if bc.__name__.lower() not in self.dependency_graph[class_name]:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As we chatted last time - now it makes more sense why you asked about scalability with lots of classes. Normally we would not be explicit on the dependency graph but construct the part as needed (in create() method we would recursively collect everything we need).

Copy link
Copy Markdown
Collaborator

@viktorb-lyft viktorb-lyft left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants