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

"dispatch" decorator needs documentation for contributors and readers of code #7189

Open
JDLH opened this issue Dec 26, 2023 · 5 comments
Open

Comments

@JDLH
Copy link

JDLH commented Dec 26, 2023

Current Behavior

Many of the NetworkX algorithm implementation functions are preceded by a decorator @nx._dispatch or variant. It is difficult for a person reading the algorithm, who just wants to understand how the Python code in the function behaves, to understand the behaviour of that decorator. How does it change the behaviour of the plain Python code in the function? How should the reader summarise in their mind the purpose of the "dispatch" decorator?

For example, a plain @nx._dispatch decorates approximation algorithm min_maximal_matching:

@nx._dispatch
def min_maximal_matching(G):
    r"""Returns the minimum maximal matching of G. ...

A @nx._dispatch() with kwarg decorates algorithm flow_hierarchy

@nx._dispatch(edge_attrs="weight")
def flow_hierarchy(G, weight=None):
    """Returns the flow hierarchy of a directed network. ...

... and dozens more.

A reader might consult the NetworkX documentation for a summary of the "dispatch" decorator. But there is no mention of it in the Introduction section or the Algorithms section.

Searching the docs for "dispatch" returns five results. Three of these are Reference documentation for the name _dispatch, which have zero(!) explanatory text. Two are Release Notes for NetworkX 3.1 and 3.2, which only mention adding the decorator, but do not explain what it does.

The module utils/backends.py has extensive text, but it is not aimed at the reader of functions decorated with "_dispatch", and it does not summarise the behaviour of the decorator for a reader of the algorithm function. Instead, it seems to be aimed at author of a backend package, explaining how the dispatching works. Part of the text seems aimed at maintainers of NetworkX test code.

The name of the decorator makes the problem worse. The name dispatch reads like a verb in the imperative form: "Dispatch this thing!" That creates an expectation that the decorator performs an action, known as "dispatch", and changes the behaviour of Python code in the decorated function. However, in the common case where no backend is present, I believe the decorator has no effect. Thus the expectation set by the imperative verb is incorrect.

Expected Behavior

There should at least two improvements.

First, there should be documentation aimed at the person reading the code of algorithm functions, summarising the effect the decorator has on the behaviour of the Python code in the function. I suspect that this could be as simple as,

'The @nx._dispatch decorator allows other libraries to take over execution of calls to this function. We call those other libraries "backends". In the common case that there is no backend, then this decorator has no effect.'

This documentation should be placed in discoverable locations. It should be in the Reference for the Utility _dispatch. A simple sentence, and link to further information, should be in the function's docstring.

Second, the decorator should be renamed. It should be a noun phrase (perhaps with adjectives) describing the function's capability, rather than being a verb in imperative form. The function is capable of being taken over by a backend, so that calls to the function are handled by the backend. Thus the function is an interface of sorts. It is open to reuse. Possible decorator names could be like:

  • @open_interface
  • @resusable_entry_point
  • @standard_frontend_func

Other documentation would also help, but gets beyond the scope of this issue. For instance, there should be an overview of the idea of "backends", and the relationship of NetworkX to those backends, as a top-level section in the documentation. There should be an explanation for the writer of an algorithm function of how to decorate the function. There should be an explanation for the writer of other packages of how to connect to NetworkX as a backend. (This partially exists, but not as a distinct section.)

Steps to Reproduce

This issue concerns documentation. Follow the links in the sections above to reproduce. There is no need to execute code.

Environment

Python version: not applicable
NetworkX version: 3.2.1

Additional context

None.

@JDLH JDLH changed the title Document @nx._dispatch Document "dispatch" decorator better for readers of algorithm functions Dec 26, 2023
@JDLH JDLH changed the title Document "dispatch" decorator better for readers of algorithm functions "dispatch" decorator is poorly documented for readers of algorithm functions Dec 26, 2023
@dschult dschult changed the title "dispatch" decorator is poorly documented for readers of algorithm functions "dispatch" decorator needs documentation for contributors and readers of code Dec 26, 2023
@dschult
Copy link
Member

dschult commented Dec 26, 2023

Thank you for this careful description of the contributor experience and for your thoughtful suggestions.
This is a relatively new feature that touches so many parts of networkx. We have been working to get it up and running and have also been making changes (as problems arise and sometimes before they arrive) to make the ride smoother. The above concerns are very helpful for us to spend some time on.

In terms of names, perhaps @allow_backends works too. But I've sometimes found the grammar of the keyword args confusing too: e.g. edge_attrs vs preserve_edge_attrs. Which do I use to do what? Does "preserve" mean "make sure they don't change during the function" -- or "make sure they get converted to the backend representation of the graph". And which refers to "all edge attributes" instead of holding a list of names of those edge attributes to be converted to the backend representation. Luckily we're not locked into certain names -- this is all supposed to still be "experimental" and can change from version to version. But I suspect it is maturing to the point where it may get more difficult to change later... :}

@JDLH
Copy link
Author

JDLH commented Dec 26, 2023

…This is a relatively new feature that touches so many parts of networkx.… Luckily we're not locked into certain names -- this is all supposed to still be "experimental" and can change from version to version. But I suspect it is maturing to the point where it may get more difficult to change later... :}

I offered this issue report in the hopes that it would arrive while names were still a little malleable, and would help you make them what you want them to be.

In terms of names, perhaps @allow_backends works too.

Yes, good idea. Though, "allow" conveys permission. I think the decorator applies to functions which are useful for backends, and structured to be dispatched to backends. So perhaps, @open_to_backends? @backend_ready?

…I've sometimes found the grammar of the keyword args confusing too: e.g. edge_attrs vs preserve_edge_attrs. Which do I use to do what? Does "preserve" mean "make sure they don't change during the function" -- or "make sure they get converted to the backend representation of the graph". And which refers to "all edge attributes" instead of holding a list of names of those edge attributes to be converted to the backend representation.…

I have the impression that declarative statements about what structure has what property will be better than imperative verb forms. That makes me favour edge_attrs over preserve_edge_attrs.

@rlratzel
Copy link

A reader might consult the NetworkX documentation for a summary of the "dispatch" decorator. But there is no mention of it in the Introduction section or the Algorithms section.

It sounds like having an overview paragraph on the Introduction page, probably right after the Data Structure section, could have helped here. I think making dispatching a more advertised, first-class concept/feature for NetworkX will go a long way in helping under-documented or not-optimally-named types be less mysterious. That said, I do appreciate good names and know how hard they can be to come up with, which brings up the next part...

The name of the decorator makes the problem worse. The name dispatch reads like a verb in the imperative form: "Dispatch this thing!" That creates an expectation that the decorator performs an action, known as "dispatch", and changes the behaviour of Python code in the decorated function. However, in the common case where no backend is present, I believe the decorator has no effect. Thus the expectation set by the imperative verb is incorrect.

@eriknw and I were discussing this and we both think @dispatchable would be better than the current name. This reads like an adjective, describing that the decorated API is capable of being or may be dispatched. I suppose it's accurate to read it as a noun too since the decorated API is a type, more specifically, it's a special kind of callable.

@aaronzo
Copy link
Contributor

aaronzo commented Mar 24, 2024

Possibly closeable after #7305 ?

@JDLH
Copy link
Author

JDLH commented Mar 25, 2024

As the original poster, and as just a user of NetworkX rather than one of its developers with a track record, my opinion is that #7305 and #7193 are a big help.

The only incremental change I might still suggest to satisfy what I wrote in this issue is to add a link from the _dispatchable first paragraph to the Backends section of Reference.

The _dispatchable first paragraph reads:

A decorator function that dispatches to func’s backend implementation based on the input graph types.

That text appears to be taken from the docstring of class dispatchable's function __new__(). The docstring of class dispatchable itself does not seem to appear in that doc section.

Just making the words "backend implementation" be a hyperlink to the Backends section of Reference would do the trick. Better yet, change it to include a crucial sentence from Backends:

A decorator function that is used to redirect the execution of a function to its backend implementation. Application code which is calling the function to get a property of a graph can disregard backend implementations; they are fundamentally performance optimizations. This decorator function dispatches to the backend based on the input graph types, and manages all the backend_kwargs.

My reasoning is this. A developer using NetworkX will read the code for an algorithm function, to understand the algorithm. The first symbol she reads is @nx._dispatchable. Searching for "dispatchable" in the docs, she finds the documentation for function _dispatchable. The first paragraph of that documentation is the first chance to explain to that developer that the decorator is not relevant to her use of the algorithm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants