Conversation
reelmatt
requested changes
Apr 14, 2020
Member
reelmatt
left a comment
There was a problem hiding this comment.
Overall, looks fantastic! One comment about updating the front-end, and then a few minor tweaks on the back-end.
I agree it does look complex, but it made more and more sense as I read through it.
Just to re-phrase some of the explanation you gave above, to see if I fully understand things:
OptionandOptionTypesare similar in that they return Parameter information.Optionkind of works as a Parameter factory, whereasOptionTypesis really just for read-only info to pass to the front-end..option_valuesis just a regular dict() that stores information passed from the form when creating/updating a Node..optionsis the "same" as.option_valuesexcept in translates the dict into a Parameter class which gets you the validation you mention. Therefore we could use either, but.optionsis more powerful/useful.
That's just me working through it in my head. Hopefully that matches with what you described above, but let me know if any of that is off.
Collaborator
Author
|
@reelmatt Those explanations are spot on. Latest commits resolve all your comments. |
reelmatt
approved these changes
Apr 14, 2020
cesaragv
approved these changes
Apr 16, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Parameterclass with subclasses forStringParameter,FileParameter, etc. These take default values, labels, and docstrings to characterize the parameter's role in the processing node.OPTIONS, a dictionary withParametervaluesOptionsandOptionTypesprovide access to those options from both the Node class and instances..option_typesfrom either the class or instance returns a dictionary representation of the defaults for use in the front end.optionsfrom the instance returns copies of the default parameters, with values taken from the instance'soption_valuesattribute. This attribute is what.optionsused to be. Accessing them through theOptionsdescriptor allows us to use the power of the Paramter classes (e.g. validation)retrieve_node_datais no longer static, because it needs access to the workflow's root_dir to look up filenames)This might seem a little overly complex at first glance, and I've been staring at it for so long that I'm no longer sure it's not. But it feels like all of the boilerplate I was able to remove from
node.pymeans it's worthwhile. Concrete classes no longer need to implement their own__init__to bubble things upward. Using descriptor-style access means the parameters must only be defined once (asNode.OPTIONS), and can be accessed flexibly depending on what the caller needs to know. Also means all the parameter metadata (type, docstring) no longer needs to be serialized in the networkx graph.