-
Notifications
You must be signed in to change notification settings - Fork 22
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
have minimal tests up-to-date and working #20
Conversation
pipeline/objects/decorators.py
Outdated
@@ -34,7 +34,9 @@ def execute_func(*args, **kwargs): | |||
) | |||
) | |||
|
|||
node_output = Variable(type_class=function.__annotations__["return"]) | |||
node_output = Variable( | |||
type_class=function.__annotations__["return"], is_output=True |
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.
I do not think this is correct, the is_output field is for when the variable is an output for the whole pipeline not a GraphNode. What's the motivation behind this?
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.
One of the errors the test caught was _output
not being found within the variables set and this was raising an error, I thought perhaps when setting the functions we wanted to define as output their output
But from your comment I gather is_output
is just for the whole pipeline not fro each node. I already have a change to fix the error while respecting this convention, just confirm if I understood correctly pls!
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.
Okay gotcha, I thought I fixed this before actually my bad!
You can register it as an output node when builder.output(...) is called, this should be working it's weird that it's not.
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.
yep, I'll review that bit to make it happen
No description provided.