feat!: unify run() and run_action() and simplify context managers

[tonyandrewmeyer]

The run_action() method (both standalone and in the context manager) are removed. This means that all events, including action events, are emitted with the same run() call, and both return the output state.

To get access to the results of actions, new action_results and action_logs attributes are added to the Context. For example:

# Scenario 6
out = ctx.run_action(Action("backup", params={"output": "data.tar.gz"}), State())
assert out.results == {"size": 1234}
assert out.logs = [..., ...]
assert out.state...

# Scenario 7
state = ctx.run(ctx.on.action("backup", params={"output": "data.tar.gz"}), State())
assert ctx.action_results == {"size": 1234}
assert ctx.action_logs = [..., ...]
assert state...

When the charm calls event.fail(), this raises an exception, in the same way that Harness does. For example:

# Scenario 6
out = ctx.run_action("bad-action", State())
assert out.failure == "couldn't do the thing"

# Scenario 7
with pytest.raises(ActionFailed) as exc_info:
    ctx.run(ctx.on.action("bad-action"), State())
assert exc_info.value.message == "couldn't do the thing"

The advantage of this is that tests that the action is successful do not need to have assertions of such, which is easy to miss.

In addition, the Context.manager and Context.action_manager methods are replaced by the ability to use the Context object itself as a context manager.

For example:

ctx = Context(MyCharm)
with ctx(ctx.on.start(), State()) as event:
    event.charm.prepare()
    state = event.run()
    assert event.charm...

The same code is also used (with ctx.on.action()) for action events.

Advantages:

• Slightly shorter code (no ".manager" or ".action_manager")
• Avoids naming complications with "manager", "action_manager" and the various alternatives proposed in refactor!: rename the context managers to event() and action() #115.

The .output property of the context manager is also removed. The context manager will still handle running the event if it's not done explicitly, but if that's the case then the output is not available. We want to encourage people to explicitly run the event, not rely on the automated behaviour - although I think it does make sense that it does run, rather than raise or end in a weird situation where the event never ran.

This replaces #115 and #118, being a combination of ideas/discussion from both, plus incorporating the unification of run/run_action discussed here, and the "action fail raises" discussed elsewhere.

Also, as drive-by changes while names are being made consistent:

• _Port becomes Port
• _RelationBase becomes (again) RelationBase

[PietroPasotti]

in principle it looks good, a couple of doubts and thoughts here and there.

[PietroPasotti]

shouldn't these types be private?

[tonyandrewmeyer]

It's a good question. @benhoyt's argument was that it's important to have the API (specifically, the .run/.run_action and .charm attributes) documented, e.g. in automatic reference docs not just in the README (or a how-to).

Users definitely shouldn't be instantiating these objects themselves, and they aren't in the top-level scenario namespace as a reflection of that. I also can't think of any cases where you'd need to use them in a type annotation.

They do 'feel' private to me - they are an implementation detail of how the context manager system works, not a part of the grander Scenario design language. I do think reference docs should include mention of the available attributes, though.

@benhoyt what if they were private, but the Context docstring had something like:

"""
Objects can also provide a context manager when tests need to interact with the charm object, for example:
    ctx = Context(MyCharm)
    with ctx(ctx.on.start(), State()) as event:
        # `event.charm` is the `MyCharm` object created to process the event
        event.run()  # When using `ctx.on.action()`, use `event.run_action()`
"""

The wording can be tidied up a bunch, but generally explaining the small API of the context manager object inside of the Context docstring, rather than having the other objects be public. I think the API is small enough (one method, one object) that people wouldn't be looking them up in reference docs much anyway, and would likely start with the Context object itself. It also avoids having both Context.run and ManagedEvent.run in the reference docs.

[benhoyt]

I don't love that the docs would have to explain the run and run_action methods just as text inside the docstring. I still think they should be broken out as real types in the docs. They are real types the user interacts with (just not their name), so it seems to me they should have first-class documentation.

[PietroPasotti]

Yeah but IMHO 'we need these types to show up in the docs' is not a good argument to have them public.
Is there no way to have them private and still convince sphinx to document them as if they were public?

Also, how's the code completion on the managers? Will the IDE warn against ctx(on.install...) as e: e.run_action()?
If not, we should try using typing.overload.

[tonyandrewmeyer]

Things are a little different now that there's just a single class and one run that always has the same signature.

IDEs/language servers don't really care about the private-ness, so if Manager is _Manager you'll still get .run and .charm auto-completion and the docstrings popping up inline.

Sphinx's autodoc functionality can be given a list of private names that should also be included. So we could have _Manager and it could still be in the reference docs.

I still don't think that anyone is going to be looking up the Manager class or its attributes/methods in reference documentation, so having it there (in addition to what's in Context's docstring) would be for completeness rather than adding actual value.

I also do feel like "there is a class called Manager" is an implementation detail that should be open to change in the future without being considered breaking. The public API is that you get an object that has run and charm attributes. I think that would most properly be documented like:

class HasRunAndCharm(abc.ABC):
    charm: ops.CharmBase
    @abc.abstractmethod
    def run(): -> State
        ...

class Context:
    def __call__(self, event, state) -> HasRunAndCharm:
        ...

But that's noisier than just having the class be public.

My preference is to have the name be private (_Manager or _EventManager) and if @benhoyt strongly wants it in the reference docs (which I think will be the case even after reading the above 😄) then adjusting the Sphinx autodoc so that it's included.

[dimaqq]

To summarise, with this change the charmer has two options:

out = ctx.run(event, state)

or

with ctx(event, state) as event: out = event.run()

Did I get that right?

Having ctx as callable in one place and an object in another is tad confusing, but I can't think of better syntax...

P.S. @tonyandrewmeyer pls update the PR description, I guess it's .run() now and not .run_action(), is it?

P.P.S. perhaps we could clarify this in the docs like this:

with ctx(event, state) as event: # bound event?

or

with ctx(event_source? event_generator?, state) as event:?

[dimaqq]

Off-topic
I really wish there was a better name than "Context" for ctx.
Frankly, I'm still unclear what sort of context it is, what gets contextualised by it...
Given that it stands for "charm execution context", I kinda with it was called "Charm".
The code could look like:

charm = scenario.Charm(MyRealCharm, meta=...)
state_out = charm.run(charm.on.startup(), state_in)

That would be easier to understand, wouldn't it?

[PietroPasotti]

Off-topic I really wish there was a better name than "context" for ctx. Frankly, I'm still unclear what sort of context it is, what gets contextualised by it...

In my head it's the charm's execution context. The filesystem it is situated in, the envvars the python process runs with (encapsulated further in the Event), the juju version...

[tonyandrewmeyer]

P.S. @tonyandrewmeyer pls update the PR description, I guess it's .run() now and not .run_action(), is it?

I'm reworking this PR to rename run_action as suggested and as discussed by charm-tech. I'll update the description to match when the code is changed.

[dimaqq]

scenario.CharmContext then? 🤔

[tonyandrewmeyer]

To summarise, with this change the charmer has two options:

out = ctx.run(event, state)

or

with ctx(event, state) as event: out = event.run()

Did I get that right?

Yes. This was the case before as well, it's just the names that have changed (and that you can now use the same API for action events as other events). state_out = ctx.run(event, state_in) is unchanged, and with ctx.manager(event, state_in) as mgr: state_out = mgr.run() is now the simpler with ctx(event, state_in) as event: state_out = event.run().

Note that the "event" that's the first argument would hardly ever be an object named "event", it would almost always be ctx.on.something(possible_args), like ctx.on.start() or ctx.on.relation(joined_relation=rel). This is a Event object, but it's roughly the same as an ops.EventSource.

The "event" that is the manager that you get back can obviously be named anything the charmer likes. If people like to stick with "mgr" (or "manager") that would be ok. It only has one attribute (.charm) and one method (.run()). I think of it as the event (and therefore the charm that's instantiated to handle the event, and .run() "runs" (emits) the event) so I like "event" (or "evt"). The ideal would be that it gave you the charm and so you'd call it "charm", but you need to be able to do the run call. See the discussion above and in the linked PRs.

(One possibility I only thought of while doing the latest changes is:

with ctx(ctx.on.start(), State()) as charm:
    charm.prepare_function()
    ctx.run(charm)
    assert charm...

But I still prefer what we have to that).

Having ctx as callable in one place and an object in another is tad confusing, but I can't think of better syntax...

Having a method instead (like we have now in Scenario 6) and either leaving it named "manager" or renaming it was the original option, but I do prefer this version (again there's lots of discussion in the links PRs). If feedback on the beta is really negative, it wouldn't be hard to change it back.

P.P.S. perhaps we could clarify this in the docs like this:

with ctx(event, state) as event: # bound event?

bound_event is a reasonable name, but it feels long.

or

with ctx(event_source? event_generator?, state) as event:?

If someone is going to put a name on the first argument, instead of doing an inline ctx.on, then I think we leave it to them to label it (I don't think this needs to be anywhere in the examples in the docs). event_source is probably reasonable.

[tonyandrewmeyer]

Off-topic

Let's move that discussion to the ops development channel or a separate issue - these PRs tend to get pretty long as-is, even when staying on topic.

[tonyandrewmeyer]

Ok @benhoyt, @PietroPasotti , @dimaqq this is ready for another round, with the much enlarged scope (I've updated the description to cover it all). I'm pretty happy with the current state, so looking forward to hearing what you all think!

[PietroPasotti]

much nicer! Few nits here and there.

Reading the readme I'm not convinced about the event name for the context manager. I'd vote for keeping mgr in the official examples :)

[dimaqq]

Looks good :)

[benhoyt]

I really like this flattened API, thanks!