Skip to content
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

Surprising behaviour with families and multiple inheritance #2570

Open
arh89 opened this issue Feb 7, 2018 · 10 comments
Open

Surprising behaviour with families and multiple inheritance #2570

arh89 opened this issue Feb 7, 2018 · 10 comments
Labels
bug? Not sure if this is a bug or not
Milestone

Comments

@arh89
Copy link

arh89 commented Feb 7, 2018

If the suite definition has tasks which inherit from multiple families, the first family is used to group the tasks in gcylc. Unfortunately, this can lead to surprising (though not necessarily incorrect!) behaviour when manually changing the state of a family through gcylc.

Example:
suite definition:

[scheduling]
    [[dependencies]]
        graph = "foo_task1 => foo_task2 => bar_task"
[runtime]
    [[root]]
        script = "sleep 10"
    [[FOO_FAMILY]]
    [[BAR_FAMILY]]
    [[foo_task1]]
        inherit = FOO_FAMILY
    [[foo_task2]]
        inherit = FOO_FAMILY
    [[bar_task]]
        inherit = BAR_FAMILY, FOO_FAMILY

This gives the following view in gcylc:

CYCLE_POINT
    FOO_FAMILY
        foo_task1
        foo_task2
    BAR_FAMILY
        bar_task

If changing the state of FOO_FAMILY, it would be reasonable to expect that the behaviour is the same as selecting all items beneath the family in the dropdown, and changing the state of those together - when the behaviour is actually to change the state of all tasks which inherit from FOO_FAMILY - ie: bar_task inherits this state. I don't think this is wrong, but from a UX point of view (especially if the user is not familiar with the suite), it can be surprising and lead to unexpected results.

@matthewrmshin matthewrmshin added this to the soon milestone Feb 7, 2018
@matthewrmshin matthewrmshin added the bug? Not sure if this is a bug or not label Feb 7, 2018
@matthewrmshin matthewrmshin self-assigned this Feb 7, 2018
@matthewrmshin
Copy link
Contributor

From the UX point of view, I think it can be considered a bug. This may or may not be easy to fix. I'll take a look soon.

@oliver-sanders oliver-sanders mentioned this issue Mar 7, 2019
5 tasks
@hjoliver
Copy link
Member

(I guess I forgot to comment on this at the time it was posted).

I can see how this could be confusing to users, but what to do about it is not exactly clear to me. We use first-parents for visualization grouping (because that creates a linear tree) but a task is considered to be a member of all families that it inherits from. This can be sometimes be useful - e.g. technical settings for all tasks that go to a particular job host would typically not be in a first-parent family for visualization, but you might want to poll (say) all jobs on that host by targeting that family with the CLI.

Possible solutions:

  1. just document more clearly and warn users to be careful using the same family both as a first and second parent (as in the example above)
  2. make family operations in the GUI (but not the CLI?) target only first-parent children rather than all children?
  3. list all children under families in the GUI, not just first-parent children? Some tasks would appear under multiple families. This might get crazy if there are grandparents etc. too!
  4. separate GUI/visualization grouping from runtime inheritance
    • a different linear (single parent) visualization setting? ("group under = "?)
    • dependency graph based grouping? (collapse sub-graphs?) - not very intuitive in the non-graph views?

Option 2. is probably the best, if we don't go as far as 4.

@hjoliver
Copy link
Member

Actually, maybe we should go as far as 4. @cylc/core - what do you think?

If we separate visualization from runtime inheritance, there will no confusion, no need to use "inherit = None" to prevent single parents beyond used for visualization, no need to use empty families just for visualization grouping.

The visualization grouping setting could itself be inherited though, like everything else under runtime.

@oliver-sanders
Copy link
Member

  1. I think would probably result in someone opening an equal and opposite issue to this one.
  1. I'm not particularly keen on the triple-use of families (runtime, graphing and visualisation) so this seems more sensible to me.

For grouping tasks in suite visualisations I think parametrisation and sub-graphs make more sense though sadly Cylc functionality is not quite there yet.

@matthewrmshin matthewrmshin modified the milestones: soon, cylc-9 Aug 28, 2019
@matthewrmshin
Copy link
Contributor

The concept of family needs a rethink, perhaps as part of #3304.

@matthewrmshin matthewrmshin changed the title gcylc - surprising behaviour with families and multiple inheritance Surprising behaviour with families and multiple inheritance Aug 28, 2019
@hjoliver
Copy link
Member

@TomekTrzeciak has raised this problem again here: #3335 (comment)

... where he made an additional suggestion:

If leading (first) and secondary families formed disjoint sets, however, the above situation would be impossible. My suggestion would be to check for this condition during suite validation and issue a warning in Cylc 7.8.x and treat it as an error in Cylc 8+.

@hjoliver
Copy link
Member

@TomekTrzeciak - just to be clear, you're suggesting there that we should check and warn (cylc-7) or fail (cylc-8) if any family is used both as a first and a second parent for different tasks?

@hjoliver
Copy link
Member

To recap:

  • the primary reason for families is multiple inheritance of runtime settings
  • we then co-opted the first-parent hierarchy for collapse/expand in visualization too, but with the consequences:
    • inelegant use of inherit = None, FOO to demote FOO to 2nd parent to avoid visualization without affecting inheritance
    • the UI somewhat misleading because operating on a family affects all the inheriting tasks, including those outside of the first-parent hierarchy

On the other hand, it is sometimes useful to be able to operate on all members of a non first-parent family (e.g. all tasks that inherit settings for a particular job platform).

@hjoliver
Copy link
Member

My preferred solution (and I think @oliver-sanders agreed above) is:

  • leave inheritance of runtime settings as-is
  • but introduce new orthogonal visualization-specific grouping settings, for Cylc 8
  • (and later on: grouping by sub-graph and parameters, but that probably has to wait for Cylc 9)

And in the meantime, for Cylc 7, we can warn on the problematic cases as suggested by @TomekTrzeciak.

@TomekTrzeciak
Copy link
Contributor

@TomekTrzeciak - just to be clear, you're suggesting there that we should check and warn (cylc-7) or fail (cylc-8) if any family is used both as a first and a second parent for different tasks?

Yes, although the solution you prefer might be even better, but it also has further ranging implications. I think on some level families and GUI groups will have to be considered jointly anyway, e.g., for task selection on the command line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug? Not sure if this is a bug or not
Projects
None yet
Development

No branches or pull requests

5 participants