New Design of VariableNamesRegistry to aid Input/Output slot sync#321
New Design of VariableNamesRegistry to aid Input/Output slot sync#321
Conversation
…pe(InputRowInfo, OutputRowInfo) and so refer directly to model.name
…le registry object
Codecov Report
@@ Coverage Diff @@
## master #321 +/- ##
=========================================
+ Coverage 95.59% 96.19% +0.6%
=========================================
Files 52 52
Lines 2542 2578 +36
Branches 330 354 +24
=========================================
+ Hits 2430 2480 +50
+ Misses 73 58 -15
- Partials 39 40 +1
Continue to review full report at Codecov.
|
…e_variable properties in VariableNamesRegistry
…ing to Variables are picked up by WorkflowTree
| # work arounds for the name validation. Putting better error | ||
| # handling into the force_bdss could resolve this. | ||
| available_variables = List(Tuple(Identifier, CUBAType)) | ||
| # Variable selected by the UI to hook model up to |
There was a problem hiding this comment.
Here we introduce the Variable class as selectable objects for MCO parameters or KPIs. These objects are generated in the VariableNamesRegistry and updated each time an input of output slot is edited.
| if self.selected_variable is None: | ||
| if self.model is not None: | ||
| self.model.name = '' | ||
| if isinstance(self.model, BaseMCOParameter): |
There was a problem hiding this comment.
Instances of type KPISpecification do not have a type trait
| self.model_change() | ||
| @on_trait_change('model.name,' | ||
| 'selected_variable.name') | ||
| def selected_variable_change(self): |
There was a problem hiding this comment.
This is an extra check, since the user should not be able to edit the model.name through the view anyway
| model=kpi, | ||
| available_variables=self.kpi_name_options | ||
| ) | ||
| for var in self.kpi_name_options: |
There was a problem hiding this comment.
During instantiation, if KPI already has a name, look for a matching Variable object to point to
| ) | ||
| variables = self.variable_names_registry.available_variables | ||
| for variable in variables: | ||
| if len(variable.input_slots) == 0: |
There was a problem hiding this comment.
Here we define our criteria for choosing a KPI - in this case no intermediate Variables are allowed (those consumed in the execution layer process)
| """ | ||
| self.model.input_slot_info[self.index].name = self.name | ||
| # Available names for input variables | ||
| available_names = List(Identifier) |
There was a problem hiding this comment.
At the moment this is unused, but could be useful in the future if allowing the user to select between existing variable names
| """Synchronises the InputSlotRow with the underlying model""" | ||
| self.name = self.model.input_slot_info[self.index].name | ||
| #: Model of the evaluator | ||
| model = Instance(InputSlotInfo) |
There was a problem hiding this comment.
The slot row names are now pointing directly to their model.name
| .kpi_view.verify_model_names() | ||
| ) | ||
|
|
||
| errors += self.workflow_view.variable_names_registry.verify() |
There was a problem hiding this comment.
Obtain errors referring to input and output slots that are raised by generating Variable objects
| #: representing the output variables produced by that execution layer | ||
| data_source_output = List(Tuple(Identifier, CUBAType)) | ||
| # DataSource that contains Variable as an output | ||
| origin = Instance(BaseDataSourceModel) |
There was a problem hiding this comment.
At the moment this trait is unused, but would be handy for creating a graphical representation of the execution layer process, since we have a reference of which DataSources are hooked up to each other
…ut or mco parameter
| """Default value for _variable_registry dictionary: split into two inner | ||
| dictionaries, holding defined (created by a DataSource output slot | ||
| and undefined (requiring to be created by a MCOParameter) Variables""" | ||
| return {'defined': {}, |
There was a problem hiding this comment.
I don't know if these are the most descriptive names (defined and undefined), so feel free to suggest alternatives
…stry dictionary keys for each Variable unhashable
…ng an empty Variable object as the default selected_variable
There was a problem hiding this comment.
Feature: The PR goes in a good direction and seems to work well at least in the easiest cases. There are some corner cases though, which are not too rare. Also I have a concern about the UI not being too intuitive: see below.
Code: Unfortunately I haven't had time to review the diff today 😞
Some topics came out during a discussion offline today and you may have already addressed them without pushing. Writing them here as memo.
- opening a saved workflow which has a DataSource that has a variable appearing both as input and as output, will trigger errors in the checks, but in
masterit would run - in the Data Source, changing the type of one variable will wipe entirely all variables and won't allow re-entering them (I need to close the wfmanager or to remove the DataSource)
- hooks can be casually and inadvertently created while typing: if "foo" is an existing input, creating a new output called "foobar" will rename "foo" to "foobar", because they become hooked mid-typing. Changing variable names shouldn't be "live".
I have a concern about the user not being able to anticipate whether a rename will have effect on other variables or not. I'm wondering whether a text box should appear (as a nudge) when initiating a edit in the DataSource tables' cells, which reads:
- if editing an Input variable: either "
This input variable is hooked to an Output from Layer \<N\>. Renaming will remove the hook" or "This input variable is currently unhooked" (not necessarily), or even "The MCO parameter linked to this input will be renamed as well" - if editing an Output variable: either "
[\<N\> input variables] [and] [a KPI] are hooked to this output variable; they will be renamed too.", with an option to "[ ]unhook inputs", or nothing
This is a general design concern - previously using redundant name/type combinations did not raise an error, but using
Should now be fixed in latest commit, was a bug in
This is now altered so that the |
Agreed, this is a concern - a new "status" box in the UPDATE: on branch |
…ources, rather than just non-intermediate ones
…pe(InputRowInfo, OutputRowInfo) and so refer directly to model.name
…on traits to slot_row in DataSourceView
… elements between two lists, based on thier shared attribute values
…ataSourceView to react to changes_slots event by identifying retained slots listen on model
3 participants
This PR further builds on work in #313 to improve UX and design aspects of the WfManager, working towards a graphical representation of the
Workflow, as suggested in #282. It introduces a newVariableclass in order to better manage the creation and editing of variables defined inDataSourceinput and output slots.Consequently, the names of each variable have persistence and can be tracked if they are changed. Additionally, it allows for the same input variable to be referenced multiple times in the
DataSourceslots, but dealt with as a single entity in the MCO.It closes #168 and closes #307
It addresses parts of #51, and is linked to #219 and force-h2020/force-bdss#213
Work Done
Variableclass defined invariable_names_registry.py, which acts as a layer of abstraction between theDataSourcetraits and those chosen as KPIs and parametersVariableNameRegistry.variable_registryis a dictionary containingVariableobjects that refer to input/output slots on theDataSources.KPISpecificationandMCOParameterobjects should now be automatically updated if these are changed on the correspondingDataSourcesMCOOptionsModelViewclasses now containselected_variableandavailable_variabletraits, rather than_combobox_values. Consequently, their views now useInstanceEditorsrather thanEnumEditors.variable_registry, only the model viewavailable_variabletraits are replaced, rather than theModelViewobjects, addressing issues raised in View state removed at every model change #51Know Issues
Error message handling fromVariableclass needs to be hooked up toWorkflowTreeVariableNamesRegistryhas redundant methods that could be removed / replacedBUG: Current state cannot handle CUBA types being changed during run timeIf either the number of slots or a slot type is changed, all of the variable names will be reset due to issues regarding View state removed at every model change #51fixed in WIP: Refactor DataSourceView to reflect changes in force-bdss #329Future Work
DataSourceModelvariable names, or choose from existing input / output slot values (maybe using aHistoryEditor.ModelViewpersistence rather than resetting objects each time an MCO KPI or Parameter is added or removed (View state removed at every model change #51)Include an error message if an input slot does not have either a defined output slot or MCO Parameter- Now included