2734 Component Graph Instantiation Error#3569
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## main #3569 +/- ##
=======================================
+ Coverage 99.7% 99.7% +0.1%
=======================================
Files 335 335
Lines 33253 33260 +7
=======================================
+ Hits 33124 33131 +7
Misses 129 129
Continue to review full report at Codecov.
|
eccabay
left a comment
eccabay
left a comment
There was a problem hiding this comment.
Fantastic job getting this up, Michael! I've left a few suggestions for changes, mostly just to follow our conventions.
docs/source/release_notes.rst
Outdated
| ------------- | ||
| **Future Releases** | ||
| * Enhancements | ||
| * Using the ``describe`` method on an un-instantiated ComponentGraph now gives a more helpful error message :pr:`3569` |
There was a problem hiding this comment.
Thanks for adding this! Personally, I would consider this change a "fix" instead of an "enhancement", since it's patching an existing bug.
Also, [this is a super nitpick and we haven't been doing a great job of maintaining the pattern recently but] generally we try to keep our changes in past tense (v.0.52.0 is a good example of that). So instead, this commend might be something closer to "Added a clearer error message when describe is called on an un-instantiated ComponentGraph"
evalml/pipelines/component_graph.py
Outdated
| # from tkinter import W | ||
|
|
||
|
|
There was a problem hiding this comment.
Can you remove these lines completely?
There was a problem hiding this comment.
Agreed, let's try to refrain from committing any unused, commented code.
evalml/pipelines/component_graph.py
Outdated
| } | ||
| ) | ||
| except TypeError: | ||
| raise TypeError( |
There was a problem hiding this comment.
I think a different error would be more useful here, a TypeError doesn't necessarily make sense for illegally calling a method. Personally, I always default to a ValueError.
| assert component.name in out | ||
|
|
||
|
|
||
| def test_describe_component_graph_TypeError(): |
There was a problem hiding this comment.
Nitpick: function names should be exclusively snake case - typeerror or type_error is just fine in this case.
| "y", | ||
| ], | ||
| } | ||
| cg_with_estimators = ComponentGraph(component_dict) |
There was a problem hiding this comment.
Perfect test! If you'd like to keep things cleaner and more concise, you can use our example_graph pytest fixture instead of defining your own component dict. You can see examples of how to use it in other parts of this file, for example here.
evalml/pipelines/component_graph.py
Outdated
| except TypeError: | ||
| raise TypeError( | ||
| "You need to instantiate ComponentGraph before calling describe()" | ||
| ) from None |
There was a problem hiding this comment.
do we need this None here? If not we can remove it!
There was a problem hiding this comment.
If I don't include from None it spits out 2 error messages rather than one weirdly enough (I'm not entirely sure why it does that).
There was a problem hiding this comment.
can you show me the stack trace if you have it?
* Throws error on describe if uninstantiated * Added test for Exception * Fixed linting and added to release_notes * Made changes that Becca and Jeremy suggested
* Tried to implement batch/pipeline timing * Unpin nlp_primitives but disallow v2.6.0 (#3574) * Unpin nlp_primitives but disallow v2.60 * Update release_notes.rst * more constraint / version updates * 2734 Component Graph Instantiation Error (#3569) * Throws error on describe if uninstantiated * Added test for Exception * Fixed linting and added to release_notes * Made changes that Becca and Jeremy suggested * Changed print to log, added function to logger * Linting is correct * Added test into test_logger.py * Added tests for batch time results * Updated release note * Tried to fix merge conflict * Added Jeremy's and Karsten's suggestions * Simplified tests and added new error check * Made it so that the dictionary always returns * Added comments and made code clearer * Fixed tests for search * Fixed release_notes * Fixed release notes * Update automl_search.py * Updated docstring Co-authored-by: Roy Wedge <roy.wedge@alteryx.com>
5 participants
Closes #2734.
After creating the pull request: in order to pass the release_notes_updated check you will need to update the "Future Release" section of
docs/source/release_notes.rstto include this pull request by adding :pr:123.