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

Introduce `types.orderOf` #97392

Open
wants to merge 3 commits into
base: master
from
Open

Introduce `types.orderOf` #97392

wants to merge 3 commits into from

Conversation

@Infinisil
Copy link
Member

@Infinisil Infinisil commented Sep 7, 2020

Motivation for this change

This introduces a new type orderOf for representing partially ordered sets (or equivalently, directed acyclic graphs)
2020-09-08_17-14

This is similar in functionality as the dag type in home-manager.

Ping @rycee @roberth

Things done
  • Wrote tests
  • Wrote docs
The contents of the DAG entry.
'';
};
after = mkOption {

This comment has been minimized.

@rycee

rycee Sep 7, 2020
Member

Could take the opportunity to change the names of after and before? E.g., wantedBy and wants to match the naming in systemd.

This comment has been minimized.

@Infinisil

Infinisil Sep 7, 2020
Author Member

Systemd also has after and before actually (see man systemd.unit), and those make more sense for a DAG than wantedBy and wants.

@roberth
Copy link
Contributor

@roberth roberth commented Sep 7, 2020

Shouldn't this be just a library function? It's a useful algorithm, but I doubt that it should be a type like this. It forces the attribute names like before and after to be generic and not very useful as shown by rycee's request to rename. However, renaming will make this type hard to reuse. Also the type description DAG of ${t} is not helpful at all. If you'd replace such a use case by an attrsOf (submodule ... ) and an assertion that calls the library function, it will enforce the same rules and we'll have a much better user experience.

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Sep 7, 2020

for representing directed acyclic graphs

I don't know why, but every time I hear about acrylic graphs I think of like nail extensions 🤣

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Sep 7, 2020

@roberth I guess I can make this a library function in addition as well, though the algorithm is really just a simple wrapper around lists.toposort.

I think it makes sense for it to be a type, because that's the most convenient way to use it. It takes care of a bunch of boilerplate:

  • The type returns an ordered list of the result already (in contrast with attrsOf), so the module author doesn't need to call the sorting function where it's used. This also saves the module author from having to throw an error for cycles.
  • The submodule type for the data, before and after options is very generic and makes sense in all DAG's. By having this predefined the module author doesn't need to create those options themselves.
@roberth
Copy link
Contributor

@roberth roberth commented Sep 7, 2020

@Infinisil Let's see if we can improve this then. My concern is that a small convenience for the module author comes at the cost of usability for, well, users.

Perhaps a usage by the module author could look like this:

type = types.dagOf { predecessors = v: v.after; successors = v: v.before; } (types.submodule { options.before = .....; ..... })
  • types.dag now uses DAG terminology, to avoid any confusion with the actual domain
  • data is gone 🎉
  • module author can now choose submoduleWith or reuse a submodule type
  • module author can write domain-specific descriptions for options like before (the current hardcoded ones are ambiguous or wrong, as can be expected to happen when you're juggling generic stuff)
  • the arguments passed here can probably be the defaults, let's see

Systemd works like this:

If unit foo.service contains the setting Before=bar.service and both units are being started, bar.service's start-up is delayed until foo.service has finished starting up.

This seems to be the inverse of what this PR is doing (unless I'm not mapping between the domains correctly??). To quote this PR's before doc:

The given entries should be ordered before this one.

Am I reading this right? All I know for sure is that the user should get to read docs like the first of the two.

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Sep 7, 2020

@roberth Oops I think I wrote that doc description the wrong way around. I'll give your idea some thought though, I think I like it!

@Infinisil Infinisil force-pushed the Infinisil:dag branch from be7175a to c9ae171 Sep 8, 2020
@Infinisil Infinisil changed the title Introduce `types.dagOf` Introduce `types.orderOf` Sep 8, 2020
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Sep 8, 2020

Changed this a bunch, now it's a lot simpler, and only requires giving a before relationship. See the tests for how such a before looks for after and before-style options.

Infinisil added 3 commits Sep 7, 2020
@Infinisil Infinisil force-pushed the Infinisil:dag branch from c9ae171 to a4d99a3 Sep 8, 2020
@Profpatsch
Copy link
Member

@Profpatsch Profpatsch commented Sep 14, 2020

I’m having doubt whether allowing arbitrary turing complete predicates (aka before) is the best way to model DAGs. There’s no way to have any static introspection in the ordering short of actually running all the before functions.

@Profpatsch
Copy link
Member

@Profpatsch Profpatsch commented Sep 14, 2020

In particular, I don’t know if you can actually have any guarantees about the result being acyclic by modeling it this way?

Also properties I would expect from a DAG, like “if B is a subtree and A is before B, then every child of B is after A” are not a given.

@roberth
Copy link
Contributor

@roberth roberth commented Sep 14, 2020

I’m having doubt whether allowing arbitrary turing complete predicates (aka before) is the best way to model DAGs. There’s no way to have any static introspection in the ordering short of actually running all the before functions.

Nix is a lazy Turing complete language, which means that behind everything that "looks like data" is a thunk that may evaluate to bottoms like exceptions or the infinite recursion you seem to want to prevent somehow.
That said, an "adjacency list" representation, like systemd.*.*.before, may lend itself to a more efficient implementation. We don't seem to have needed a faster implementation of toposort, as far as I know.

In particular, I don’t know if you can actually have any guarantees about the result being acyclic by modeling it this way?

The type converts the definitions to a list that is sorted according to the before function. Such an outcome is not possible when the input is cyclic, so the non-exception result is guaranteed to be acyclic.

<replaceable>elemType</replaceable> }
</term>
<listitem>
<para>

This comment has been minimized.

@roberth

roberth Sep 14, 2020
Contributor

The fact that types can produce a result that is of a very different shape is non-obvious but essential to the understanding of orderOf, so it seems like a good idea to briefly explain this aspect of orderOf first.

<term>
<varname>types.orderOf</varname> {
<replaceable>before</replaceable> ? a: b: false,
<replaceable>elemType</replaceable> }

This comment has been minimized.

@roberth

roberth Sep 14, 2020
Contributor

Suggested change
<replaceable>elemType</replaceable> }
<replaceable>wrappedType</replaceable> }

elemType would have been the entire attrsOf foo for example, despite the name hinting that it'd only be elemType = foo.

@Profpatsch
Copy link
Member

@Profpatsch Profpatsch commented Sep 14, 2020

The type converts the definitions to a list that is sorted according to the before function. Such an outcome is not possible when the input is cyclic, so the non-exception result is guaranteed to be acyclic.

Yes, but it would mean an “infinite recursion” in practice, right? Which is not very user-friendly.

Plus, this is still an issue in my mind:

Also properties I would expect from a DAG, like “if B is a subtree and A is before B, then every child of B is after A” are not a given.

@roberth
Copy link
Contributor

@roberth roberth commented Sep 14, 2020

Yes, but it would mean an “infinite recursion” in practice, right? Which is not very user-friendly.

toposort produces a distinct attrset describing the cycle, which is turned into an exception by types.orderOf.

It would be nice to have a check function though.

Also properties I would expect from a DAG, like “if B is a subtree and A is before B, then every child of B is after A” are not a given.

That's what this type is all about, to ensure that either it will be the case or an error is raised. Perhaps you could formulate your concern differently or provide an example that is handled poorly by this type and tell us what you would expect it to do instead.

let
nodeAttributes = (attrsOf elemType).merge loc defs;
entries = mapAttrsToList nameValuePair nodeAttributes;
sortedEntries = toposort before entries;

This comment has been minimized.

@Profpatsch

Profpatsch Sep 14, 2020
Member

toposort is O(n^2), so I’m kinda afraid this might slow down the module system even more if it’s used in a few places.

This comment has been minimized.

@Infinisil

Infinisil Sep 14, 2020
Author Member

I'm not sure if it's possible to implement toposort more efficiently. However I can look into whether it's possible to implement a different interface more efficiently, e.g. something that isn't based on a before function but on a successor mapping instead, e.g. graphToposort { foo = [ "bar" ]; bar = []; } representing foo having a directed edge to bar. In theory, topo sorts can be O(|N| + |E|), though that might not be implementable in Nix.

@Profpatsch
Copy link
Member

@Profpatsch Profpatsch commented Sep 14, 2020

That's what this type is all about, to ensure that either it will be the case or an error is raised.

Which is what I’m not sure about. I looked at the implementation of toposort, but I don’t know what happens if arbitrary predicates are passed to it. Maybe it’s fine.

@Profpatsch
Copy link
Member

@Profpatsch Profpatsch commented Sep 14, 2020

Maybe more importantly: Do we have applications for this type?

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Sep 14, 2020

Ignoring backwards compatibility, this could be used for:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.