-
Notifications
You must be signed in to change notification settings - Fork 27
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
added family grouping plus family and cycle collapsing #1810
base: master
Are you sure you want to change the base?
Conversation
Still to do (in order of priority).
|
I think this is a problem that warrants recursion as it's tricky to unroll as an iterative loop. Here's an idea of what that could look like (Python syntax):
from random import random
TASKS = {
'foo': {
'name': 'foo',
'parent': 'FOO',
},
'FOO': {
'name': 'FOO',
'parent': 'root'
},
'bar': {
'name': 'bar',
'parent': 'BAR1',
},
'baz': {
'name': 'baz',
'parent': 'BAR2',
},
'BAR1': {
'name': 'BAR1',
'parent': 'BAR',
},
'BAR2': {
'name': 'BAR2',
'parent': 'BAR',
},
'root': {
'name': 'root',
'parent': None,
},
}
TREE = {
'root': {
'FOO': None,
'BAR': {
'BAR1': None,
'BAR2': None,
},
},
}
def add_subgraph(dotcode, pointer, graph_sections):
for key, value in pointer.items():
dotcode.append(
f'subgraph cluster_{str(random())[2:]} {{'
f'\nlabel = "{key}"'
)
if value:
add_subgraph(dotcode, value, graph_sections)
if key in graph_sections:
dotcode.extend(graph_sections[key])
dotcode.append('}')
return dotcode
def get_dotcode(tasks):
graph_sections = {}
for task in tasks.values():
parent = task['parent']
if not parent:
continue
section = graph_sections.setdefault(parent, [])
section.append(f'{task["name"]} [title="{task["name"]}"]')
dotcode = ['digraph {']
add_subgraph(dotcode, TREE['root'], graph_sections)
return dotcode
for item in get_dotcode(TASKS):
print(item) digraph {
subgraph cluster_23300787190407446 {
label = "FOO"
foo [title="foo"]
}
subgraph cluster_5025488657295563 {
label = "BAR"
subgraph cluster_2135762450670372 {
label = "BAR1"
bar [title="bar"]
}
subgraph cluster_4413670667138756 {
label = "BAR2"
baz [title="baz"]
}
BAR1 [title="BAR1"]
BAR2 [title="BAR2"]
} I haven't taken cycles into account in this solution, you'll need to add a This solution will also add entries for families which have no tasks, so, you'll need some fancy logic for removing empty families, and any families that contain only empty families. |
21472f9
to
e45ccec
Compare
f1540f6
to
676466f
Compare
src/views/Graph.vue
Outdated
// ---------------REMOVE NODES BASED ON CYCLE POINT------------ | ||
if (!this.allChildrenLookUp[indexSearch.id]) { return } | ||
this.allChildrenLookUp[indexSearch.id].forEach((config) => { | ||
if (config.name !== indexSearch.name && config.name !== '01') { |
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.
What is the !== '01
check doing?
src/views/Graph.vue
Outdated
// its the node has been removed/collapsed | ||
const isRemoved = !removedNodes.includes(a.name) | ||
// is not numeric | ||
const isNumeric = !parseFloat(a.name) |
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.
What is the isNumeric
check for?
Is this here to filter out jobs?
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.
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.
So I could rename this to is retry @oliver-sanders ?
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.
Ach right, these are jobs rather than tasks.
The data store has a hierarchy like this:
- user
- workflow
- cycle
- task
- job
- task
- cycle
- workflow
We can avoid the need to filter out job nodes, by not looking at the children of task nodes.
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.
Ive made a change to the data being returned from allChildrenLookUp
7f6a89c
so there shouldnt be any jobs in it now... and this isNumeric
check isn't needed
src/views/Graph.vue
Outdated
}`) | ||
const graphSections = {} | ||
Object.keys(cycles).forEach((cycle, iCycle) => { | ||
const indexSearch = Object.values(this.cylcTree.$index).filter((node) => { |
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.
This is searching over the full cylcTree.$index
, this is the complete data store which also contains nodes for other workflows.
We only need to iterate over the cycles in this workflow, i.e. Object.keys(cycles).forEach((key, i) => {
.
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.
Done in commit 139ee67
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.
Cheers.
There are a couple of more instances of this, e.g, this forEach is looking through all of the objects in the store which includes other workflows and job nodes which we aren't interested in:
Object.keys(this.cylcTree.$index).forEach((itemName) => {
We could do something like this instead:
for (const workflow of this.workflows) {
for (const cycle of workflow.children) {
for (const task of cycle.children {
// stop here (don't look at jobs)
}
}
}
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 think this should now be resolved in 51fb8df
I cant see any other examples of forEach
on this.cylcTree.$index
This comment was marked as resolved.
This comment was marked as resolved.
const edges = this.getGraphEdges() | ||
let edges = this.getGraphEdges() | ||
|
||
// ---------------------------------------- |
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.
Collapsing nodes - adding and removing nodes and edges.
}) | ||
} |
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.
Grouping nodes
src/components/cylc/ViewToolbar.vue
Outdated
data () { | ||
return { | ||
selectedItems: { | ||
groupFamily: [], | ||
collapseFamily: [], | ||
collapseCycle: [] | ||
} | ||
} | ||
}, | ||
mounted () { | ||
if (this.groups[0].controls.length >= 6) { | ||
this.selectedItems.groupFamily = this.groups[0].controls[7].value | ||
this.selectedItems.collapseCycle = this.groups[0].controls[8].value | ||
this.selectedItems.collapseFamily = this.groups[0].controls[9].value | ||
} | ||
}, | ||
|
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.
src/components/cylc/ViewToolbar.vue
Outdated
data () { | ||
return { | ||
selectedItems: { | ||
groupFamily: [], | ||
collapseFamily: [], | ||
collapseCycle: [] | ||
} | ||
} | ||
}, | ||
mounted () { | ||
if (this.groups[0].controls.length >= 6) { | ||
this.selectedItems.groupFamily = this.groups[0].controls[7].value | ||
this.selectedItems.collapseCycle = this.groups[0].controls[8].value | ||
this.selectedItems.collapseFamily = this.groups[0].controls[9].value | ||
} | ||
}, | ||
|
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.
Is this any better?
mounted () {
if (this.groups[0].title == 'Graph') {
const getControlValue = (groups, controlKey) => {
return groups[0].controls.find(control => control.key === controlKey).value
}
this.selectedItems.groupFamily = getControlValue(this.groups, 'groupFamily')
this.selectedItems.collapseCycle = getControlValue(this.groups, 'collapseCycle')
this.selectedItems.collapseFamily = getControlValue(this.groups, 'collapseFamily')
}
},
src/views/Graph.vue
Outdated
}`) | ||
const graphSections = {} | ||
Object.keys(cycles).forEach((cycle, iCycle) => { | ||
const indexSearch = Object.values(this.cylcTree.$index).filter((node) => { |
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.
Done in commit 139ee67
src/views/Graph.vue
Outdated
// its the node has been removed/collapsed | ||
const isRemoved = !removedNodes.includes(a.name) | ||
// is not numeric | ||
const isNumeric = !parseFloat(a.name) |
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.
So I could rename this to is retry @oliver-sanders ?
|
||
data () { | ||
return { | ||
selectedItems: {} | ||
} | ||
}, |
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.
This seems to be unused?
@@ -659,7 +1533,8 @@ export default { | |||
} | |||
|
|||
const cycles = this.getCycles(nodes) | |||
|
|||
this.latestCycle = Object.keys(cycles)[0] |
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.
latestCycle
unused?
let removedEdges = [] | ||
|
||
// For all cycles... | ||
this.cycleArrayStore.forEach((cycle) => { |
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.
Style note for future: generally I think we prefer for...of
rather than Array.prototype.forEach
:
- You can't break out of
forEach
apart from throwing an error - Debugging is more natural as "step over" goes into
for
loops, but skipsforEach
callbacks
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 have been leaning towards for of
rather than forEach
.
From a quick search, it looks like forEach
predated for of
. for of
seams cleaner to me and appears to be more efficient.
const store = [] | ||
this.cycleArrayStore.forEach((name, id) => { | ||
store.push({ | ||
id, | ||
name | ||
}) | ||
}) | ||
return store |
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.
const store = [] | |
this.cycleArrayStore.forEach((name, id) => { | |
store.push({ | |
id, | |
name | |
}) | |
}) | |
return store | |
return this.cycleArrayStore.map((name, id) => ({ | |
id, | |
name, | |
})) |
Partly addresses issue #1130
The grouping of nodes by cycle point is completed in this pr #1763
----Notes on work----
Some ideas for a unified approach to grouping/collapsing cycles/families. I'm suggesting unifying the handling of cycles and families (note, cycles represent the "root" family so they are essentially the same thing).
Grouping/Ungrouping - Drawing dashed boxes around a cycle/family.
Collapsing/Expanding - Reducing a family down to a single node.
Limitations of the Cylc 7 approach:
Note, for simplicity, this approach groups/collapses all instances of selected families rather than managing this at a per-cycle level. I think this is probably more aligned with expectations, but does represent a minor limitation, e.g. there's no ability to collapse all but one cycle. The ability to expand/collapse specific cycle instances would be a reasonable enhancement.
Design Sketch
Had a quick discussion on this (more to come):
Check List
CONTRIBUTING.md
and added my name as a Code Contributor.setup.cfg
(andconda-environment.yml
if present).CHANGES.md
entry included if this is a change that can affect users?.?.x
branch.