Start of Endpoint implementation for easier interface layers

[johnsca]

Beginning of the implementation for the proposal for new style interface layers using the Endpoint base, eschewing the reliance on @hook in favor of @when everywhere, and removal of the "conversations" metaphor. This implements up to and including the "Managed Flags" section.

fixes #107
fixes #88
fixes #61
fixes #102

[johnsca]

(3) RelationBase: Managed Flags

[stub42]

Endpoint._startup should not be called in 'restricted' mode, or metrics and similar hooks are going to become heavyweight. Maybe the charms.reactive module should grow a global restricted_mode flag, initialized by main, and Endpoing._startup checks for it.

[johnsca]

_run_atstart isn't called in restricted mode, so it won't be.

[johnsca]

(3) RelationBase: Relation and Unit collections

[johnsca]

(3) RelationBase: Sending and Receiving Relation Data

[battlemidget]

Does this need to be updated to reflect the name of the function argument?

[battlemidget]

Here to?

[battlemidget]

best module ever

[battlemidget]

From what my limited knowledge of the code does it LGTM

[kwmonroe]

+1 on the recent docstring commit -- really nice job explaining wth is going on @johnsca!

[johnsca]

(2) Endpoint: Documenting Interface Layers

[jamesbeedy]

@johnsca this is really great. One question, how might a peer interface take advantage of this new approach?

[galgalesh]

I finally found time to look at this PR, I'm really excited about it!

I pushed some docstring changes, please let me know if I'm writing stuff that's incorrect. I feel that "publishing" is a good description of the behavior of relation-set, so I used that to describe the behavior of the send dictionary. What are your thoughts about changing the name of send to to_publish? I feel that explains its behavior better and it addresses the concern we had that send is not true to what actually happens behind the scenes.

[galgalesh]

I'd like some more time to look at it, show it to the team and write an interface myself, but what I've seen looks very good!

[galgalesh]

Maybe call it EndpointInterface to make explicit that this isn't an endpoint itself but the interface of an endpoint. Multiple endpoints of the same charm can use the same interface, and thus the same EndpointInterface class, right?

[galgalesh]

Shouldn't this be endpoint_name?

[johnsca]

My thinking here (and for the Endpoint vs EndpointInterface above) was that the class represented the endpoint interface but specific instances represent actual relations (since they have an associated ID). But I'm also feeling like that distinction might be too esoteric, and really the name is associated with the endpoint itself, not a specific relation. So +1 on this and the above class name change.

[galgalesh]

It seems like this could be used to "document" the "public" flags of an interface layer, is that the case? If so, can you provide an example of how you would do that?

[johnsca]

This was really more of a helper for populating the {endpoint_name} (I'm inclined to change that to match the method name, discussed above) portion in a consistent way, and to provide a shortcut if you didn't want to type that bit out all the time (probably a bad idea?). I'm not sure how it would be used to document public flags for the layer since they happen at runtime; can you explain your thoughts there more?

I had also toyed around with the idea of having some class-level structure to pre-define the flags, but you still couldn't populate the {endpoint_name} portion until runtime, so I'm not sure how we could make that work. But that would certainly give us a great way to be able to add doc strings for the flags themselves, perhaps even integrated into the definition itself.

[galgalesh]

What do you think about refactoring send_json -> send and send -> send_raw? This will make it more clear that the json variant should be used unless you need lower-level access to be compatible with an existing interface.

(to_publish and to_publish_raw if you agree on that change too)

[johnsca]

+1 on both.

[galgalesh]

What do you mean with

if you're uncertain which is used by an interface layer

?

[johnsca]

Well, old-style relation instances aren't populated into the global context (because they depend on which specific flag you're handling), so you have to use relation_factory (or the terrible ambiguous handler args) to get access to them. And whether an interface layer uses EndpointInterface or RelationBase is an implementation detail that the charm author shouldn't care about. This will be problematic during the transition, so I'm not sure how to handle it gracefully.

[galgalesh]

How do we handle the case when a handler doesn't accept instances but the decorator wants to pass one? (a "new" handler reacting to a flag of an "old" interface)

[johnsca]

I added introspection logic to the invocation code so that, if the handler doesn't take any arguments, it will do the right thing and not pass them. You'd then have to use relation_factory or context.endpoints to access them, depending on how they're implemented (ugh). Though, the decorators will also pass the args through for EndpointInterface flags as well, so we'll probably end up having to continue to do that until everything is converted and then we'll still have the huge amount of inertia to move away (ugh).

[johnsca]

@jamesbeedy I answered your question on IRC, but for posterity here...

Peer relations are handled exactly the same as other relations, with the only difference being you only need one class, in a peers.py file, rather than two (in requires.py and provides.py). The class itself will be the same, just publishing data and setting flags based on what data has been published. Also, peer relations are guaranteed to only ever have a single relation ID, which makes the helper methods (like all_units) particularly useful for them.

[jamesbeedy]

This is great. Thanks @johnsca

[stub42]

Looking good!

[stub42]

I would go further here. If a charm starts relying on these flags directly, then it stops the interface layer from changing how it manages them. The implementation of the relation has moved from the interface layer into the charm.

"These flags should only be used by the decorators of the endpoint handlers. While it is possible to use them with any decorators in any layer, the endpoing.* flags should be considered internal, private implementation details. It is the interface layers responsibility to manage and document the public flags that make up part of its API."

I would also be interested in the use cases for not having the framework manage the changed and departed flags automatically. Allowing them to persist beyond the current hook will cause problems, because the flag is implicitly tied to a particular remote unit and that remote unit will be gone in future hooks, or even worse may point to an incorrect unit with the potential to cause all sorts of mischief (eg. if you manage to defer a handler waiting on a departed flag until a future hook, the remote unit may actually be a unit that is just joining!).

[stub42]

That is a complex enough description to warrent a short example. Using 'namespace' here is confusing, as to me namespaces are accessed using attribute lookup but in this context dictionary lookups are needed since relation names can contain hyphens, so maybe a reference to the NormalizingNamespace in use here (?) is needed.

I think this may also be incorrect, and it should be context.endpoints rather than context ? 'context' is a pretty awful name, with no indication of what it contains because it is... umm... context sensitive. I'm not sure why context is buried away in helpers when it seems to be a data structure forming the public charms.reactive API, rather than an actual helper.

[johnsca]

I'm starting to wonder about the viability of the context pattern for this. As you point out, the name is a bit generic, it requires magic translation of the endpoint name to a valid attribute name, and it ends up exposing the internal implementation (whether the interface layer uses EndpointInterface or RelationBase) by virtue of whether a given endpoint can be accessed via the context or not (although this last is only an issue during the transition, that transition is likely to be long).

We'll probably need to go back to recommending relation_from_flag only.

[stub42]

Is :meth: correct markup here, since it is a property? I'd also just say relations rather than the full charms.reactive...Endpoint.relations path, since this isn't a class property. But I don't know this markup well.

[galgalesh]

This is correct markup and it gets formatted as relations, the full path isn't shown in the text but is used to create a url to those docs. PS: HACKING.md explains how you can build the docs locally.

[galgalesh]

But :attr: is better because then it doesn't show the () after the name. Fixed.

[stub42]

This magic may deserve a comment. Why might I get flag without a '.', and why return None rather than an error?

[johnsca]

This is just to match how RelationBase.from_flag and MinimalRelationBase.from_flag return None when a relation instance can't be derived from a flag. The only difference being that EndpointInterface.from_flag could return an instance even if the flag isn't set. +1 to adding a comment.

[stub42]

And we can schedule the removal of the changed* and departed flags here too, as they will stop being meaningful at the end of the hook when the remote unit changes.

[stub42]

It turns out that relation ids are unique integers, and Juju is starting to report them as such in things like the new network-get tool. Perhaps we can convince Juju to stop doing that and creating confusion where none is needed. If we stick with KeyList here, I think it means that as far as charms.reactive is concerned relation ids will always be relname:int. Which is probably the sane thing to do, since relname:int is unambiguously a string while just the number could be a str or int.

[johnsca]

I do think we should push back on having relation IDs reported as plain numbers, and I'm +1 to normalizing it in the framework if Juju does report them that way.

On the other hand, though, from the charm author's perspective, I would prefer relation IDs to be an implementation detail that they don't need to worry about, as much as possible.

[stub42]

Its annoying that this is public. It is exposed in the public API that will be used by the charm, but is really intended for use by the interface implementation. Should we be using __ for the internal API, _ for the interface API to be used by subclasses, and public for the endpoing API to be used by the charm? Probably not if this is the only case, as the solution is worse than the (arguable) problem.

[stub42]

How do you know what the relation_name is from the perspective of the remote unit? Or is this just repeating this ends relation_name twice? I would have thought we want the relation id here rather than the two relation names.

This will fail if the relation name (or relation id) contains a period. I don't know if that is possible.

[stub42]

Yeah, it will be annoying if we need to support relname:x and x at every entry point :-( So lets just say relation ids are always relname:x

[stub42]

Should charms.reactive.altrelations be renamed charms.reactive.endpoints ?

[stub42]

The weakref stuff is probably noise to users, a charms.reactive main() will keep the relation around. Consider moving it to a comment to avoid polluting the API docs. Or just drop the weakref and let Python3 deal with the circular reference eventually.

[stub42]

This is sorting by relation id as a string, which will put db:10 before db:2. Similarly the unit_name, which you claim later is order by unit number. Consider key=lambda i: (int(':'.split(i.relation.relation_id)[1]), int('/'.split(i.unit_name)[1]))

[johnsca]

Good catch, thanks.

[stub42]

yer, if you want numerical order you need to do it above.

[stub42]

Should suffix be optional? Its an internal method, so making it a required argument will help catch any call sites we missed.

[stub42]

Not sure why you want _CONSUMED_FLAGS here instead of _consumed_flags. All-caps is generally reserved for constants in Python. Similarly, _HANDLERS below.

[johnsca]

Per @galgalesh's request, since the http interface is used as the example in the docs, I updated the http interface to use Endpoint in this PR: juju-solutions/interface-http#10