automl / amltk

A build-it-yourself AutoML Framework
https://automl.github.io/amltk/
BSD 3-Clause "New" or "Revised" License
68 stars 6 forks source link

fix(Node): Ensure that parent name does not conflict with children #248

Closed eddiebergman closed 10 months ago

eddiebergman commented 10 months ago

This was causing an issue when configuring, where essentially the parent would steal the configuration of its child.

This problem came about where I had a pipeline named as the estimator, which could be a pretty easy mistake to make.

Sequential(
    ...,
    ...,
    Component(
        MLPClassifier,
        name="mlp_classifier",
    ),
    name="mlp_classifier",
)

I thought it only mattered that all children of a node must have unique names and that would be enough but nope, both the parent and it's children must be uniquily named too.

Traceback (most recent call last):
  File "/home/skantify/code/exps/one.py", line 12, in <module>
    from exps.pipelines import knn_classifier, mlp_classifier, rf_classifier
  File "/home/skantify/code/exps/src/exps/pipelines.py", line 176, in <module>
    mlp_classifier = Sequential(
  File "/home/skantify/code/amltk/src/amltk/pipeline/components.py", line 437, in __init__
    super().__init__(
  File "/home/skantify/code/amltk/src/amltk/pipeline/node.py", line 313, in __init__
    raise DuplicateNamesError(
amltk.exceptions.DuplicateNamesError: Cannot have a child node with the same name as its parent. self.name='mlp_classifier' child.name='mlp_classifier'

What's nice about this PR, it can just be explicitly done in the Node init and it removes the extraneuous checks from both Seqeunce and Choice who were basically doing the same thing.