Have component_graph support ComponentBase subclasses instead of instances #850
Conversation
dsherry
changed the title
Codecov Report
@@ Coverage Diff @@
## master #850 +/- ##
==========================================
+ Coverage 90.31% 99.69% +9.38%
==========================================
Files 195 195
Lines 7740 7745 +5
==========================================
+ Hits 6990 7721 +731
+ Misses 750 24 -726
Continue to review full report at Codecov.
|
| for component in component_graph: | ||
| component_parameters = proposed_parameters.get(component.name, {}) | ||
| init_params = inspect.signature(component.__class__.__init__).parameters | ||
| component_graph = [handle_component_class(c) for c in pipeline_class.component_graph] |
There was a problem hiding this comment.
We do this in at least a couple places in our codebase. Could be nice to define a method for it. Or override PipelineBase.component_graph to return a handled version by default.
| raise MissingComponentError(err) from e | ||
|
|
||
| component_class = component.__class__ |
There was a problem hiding this comment.
This line in particular is great to get rid of. Before this PR, we were calling handle_component to get a component instance, then calling __class__ to get the component class, then using that to create a fresh component instance below. Now, we just get the class and create the instance.
| @@ -27,19 +32,28 @@ def test_all_components_core_dependencies_mock(): | |||
| assert len(all_components()) == 17 | |||
|
|
|||
|
|
|||
| def test_handle_component_names(): | |||
| def test_handle_component_class_names(): | |||
There was a problem hiding this comment.
Direct unit test coverage for handle_component_class
dsherry
force-pushed
the
ds_523_init_components_pipelines
branch
from
7745a61
to
25368c2
Compare
…, not ComponentBase subclass instance. Update handle_components impl to work with classes.
dsherry
force-pushed
the
ds_update_handle_components_class
branch
from
0dd58b4
to
8bcf35d
Compare
Problem
Currently, pipelines'
component_graphattribute accepts a list ofstrandComponentBasesubclass instances, and the helperhandle_componentstandardizes to an instance. However,component_graphis static, so it doesn't make sense to have component instances saved in that static attribute.This isn't a bug at the moment, because we never access the instances directly, we just grab the class and use that to create the actual instance during pipeline instantiation. However, it makes our core pipeline code more confusing and could lead to bugs down the road.
Fix
This PR updates
component_graphto accept a list ofstrandComponentBasesubclasses, but no longer instances. And the helperhandle_component_class(I renamed the old function) standardizes to a class, not an instance.