0.12 cannot use context for state handling

[presidento]

------------------
@ctask
def a(ctx):
    ctx['a'] = 123
    print('a')

@ctask(a)
def b(ctx):
    print(ctx['a'])
    print('b')
------------------
$ > invoke b

Expected result: a 123 b (it works with Invoke 0.11.1)
Actual result: the following exception:

Traceback (most recent call last):
  File "/home/matefarkas/work/bsp-install/bin/inv", line 9, in <module>
    load_entry_point('invoke', 'console_scripts', 'inv')()
  File "/home/matefarkas/work/invoke/invoke/program.py", line 270, in run
    self.execute()
  File "/home/matefarkas/work/invoke/invoke/program.py", line 379, in execute
    executor.execute(*self.tasks)
  File "/home/matefarkas/work/invoke/invoke/executor.py", line 114, in execute
    result = call.task(*args, **call.kwargs)
  File "/home/matefarkas/work/invoke/invoke/tasks.py", line 113, in __call__
    result = self.body(*args, **kwargs)
  File "/home/matefarkas/work/invoke/tasks.py", line 15, in b
    print(ctx['a'])
  File "/home/matefarkas/work/invoke/invoke/config.py", line 110, in __getitem__
    return self._get(key)
  File "/home/matefarkas/work/invoke/invoke/config.py", line 113, in _get
    value = self.config[key]
  File "/home/matefarkas/work/invoke/invoke/config.py", line 110, in __getitem__
    return self._get(key)
  File "/home/matefarkas/work/invoke/invoke/config.py", line 113, in _get
    value = self.config[key]
KeyError: 'a'

As I know, since 0.12 Invoke does not remembers the state changed in pre tasks.

[bitprophet]

I think we had to add some additional cloning of contexts, don't have time to git blame/log now but it should make that clear.

Having "clean" contexts handed to tasks is sort-of an intention, but obviously sharing state between tasks is also a core need, so if this change was to effect something else we'll have to figure out how to reimplement the data sharing functionality.

[presidento]

Tomorrow I will try to find the exact point when it gots wrong. Thank you for the answer.

[presidento]

This bug was introduced in commit c79d80b where config cloning and other config-related tasks were moved from execution time to expand time.

Removing clone doesn't help, because it breaks the per-task configuration.

[presidento]

Here is an idea: why are state handling and config mixed up? They are very different. I mean, we could use ctx.config for accessing the (immutable) config and ctx.state for preserving a state between tasks. (Both can be wrapped/proxied by ctx to keep the existing functinality.) And we could use config as it is now implemented, and state as it was in the previous implementation.

[bitprophet]

IIRC they're interwoven mostly for convenience's sake and/or following design lines from Fabric 1, and I think there might actually be tickets or at least comments with that same idea in them, "should we separate core config and user data?".

The core problem of "which mutations should 'bubble up' to the caller and affect subsequent code, if any" isn't an easy one. Some users/user cases assume that tasks' state changes will always affect subsequent tasks (it's a primary method of doing 'setup' in Fabric 1 for example - fab set_hosts_from_a_database:poolA,poolB actually_do_things_here).

Others assume by default that each task stands alone and starts with a 'fresh' environment; and certainly, the opportunity for difficult debugging and accidental mutation / "state bleeding" exists even if one philosophically believes that the context should be fully mutable.

(Part of this is that my software seems to disproportionally attract newbies and so I start to approach design decisions with "what will help prevent large classes of support request?" - despite Python's general philosophy of "treat your users like adults" 😞)

The tl;dr here is "this is hard, there is no single right answer and any decision is going to leave some users out in the cold, I need to think on it more and figure out the best way to let users override whatever default we end up with". I think that might mean breaking up these parts of Executor even more, so that it's easier for us to provide a handful of Executor subclasses users can select at will (aside from writing their own, of course).

[presidento]

Hi Jeff, thank you for the detailed answer.
I am thinking of implementing the state handling outside of Invoke using well wrapped common global variables. Is it a bad idea if we don't (and won't) use parallelized execution?

[bitprophet]

If you know your use case won't do any parallelization, and you're willing to deal with whatever debug unfriendliness arises from it (which for an internal or small codebase, may not be much) - then yea, globals work fine. All of Fabric 1 is predicated heavily on them and besides those two downsides, it's functional.

In my case, I need some clean solution to parallelization for Invoke because a) some users of Invoke itself have the need for "local" parallel execution, even if I personally don't, and b) I do need it for Fabric 2, being built on top of Invoke.

So there needs to be some way of preventing state bleed; but whether it's the default or not, may vary, and I'd like users to have the flexibility of opting in or out of such strictness.

[bitprophet]

Anyway I'll leave this open until I have a chance to give it a good think; thanks again for the feedback!

[tuukkamustonen]

Just bumped into this. I was happily doing state sharing through Context in 0.11.

When would a person modify the Context if not to share the state to another task? To move around variables within a single task, there's no need to touch context (and potentially mess up things). @bitprophet Can you give an example where a person would touch the ctx and not expect the change to propagate to another task?

[bitprophet]

@tuukkamustonen There's a couple reasons which I touched on earlier but I only just now refreshed my memory. It's semi outlined in this comment on #228.

The tl;dr is that before, we generated one config object (Contexts generally act as a proxy for one of these), and it got served up to all tasks scheduled for execution, effectively like this pseudocode:

# Say the user did 'invoke taskA taskB'
tasks = ['taskA', 'taskB']
config = Config()
tasks_to_execute = [(x, config) for x in tasks]
for task, config in tasks_to_execute:
    execute(task, config)

Thus, when you have 2+ tasks, and they modify the config object in any way, those modifications "bleed" into subsequent task executions - because the config object both tasks are receiving is the same Python object.

With #228 we had to update things so each 'copy' of a task being parameterized got its own cloned Context, because otherwise the task-expansion mechanisms end up doing this sort of thing:

# inv taskA --hosts=host1,host2,host3
tasks = ['taskA']
hosts = ['host1', 'host2', 'host3']
config = Config()
tasks_to_execute = []
for host in hosts:
    config['host'] = host
    tasks_to_execute.append((task, config)) 
for task, config in tasks_to_execute:
    execute(task, config)

If you squint you can see the problem: we end up executing taskA on host3 3 times in a row.

This was solved by inserting a task_config = config.clone() prior to tweaking config['host'] (essentially). Nice and clean! but also torching intentional config/context sharing between tasks, as per this ticket.

---

In the time it took me to rethink all this and write it up, it occurs to me that we could maybe have our cake & eat it too, by shuffling the order of operations around a bit.

The above problem comes up because the steps are basically:

• examine inputs (pre/post tasks as well as parameterization)
• use them to expand out to a single list of tasks-to-execute, which are Call objects that just wrap a task and a context/config together (and CLI args)
• perform deduplication and similar things on that list
• walk the final resulting list, executing the remaining Calls

We have to perform the config cloning because the config generation is occurring at the expansion step, not the execute step, which led to the bug in parameterization. That's done at least partially for convenience's sake or just because that's how it was written originally.

If we're lucky, that's all it is - happenstance - and we could change things a bit so the "bake parameterization info into the Config" step occurs inside the final loop and not before. Presto, single globally shared/inherited/passed-around Config/Context object, but parameterization still works. I will look into this soon to see if it's feasible.

---

That aside! One of the other benefits of "clean" cloning of contexts (and one reason why I didn't view the above gobbledygook as being anything but a positive change) is it prevents bugs like the below, where calling out to other parts of the code you may or may not control (or may simply not remember are making mutations) can easily bite you in the ass:

@ctask
def build(c):
    # Unbeknownst to us, clean() is going to modify core config settings.
    clean(c)
    # Here, we aren't expecting anything but default config values...but!
    # The below will run silently because clean() set hide=True and we haven't set it back.
    c.run("something where we expect some stdout or stderr")

@ctask
def clean(c):
    # We always want to run silently no matter what our body below happens to be doing
    c.config['run']['hide'] = True
    if c.run("test -d build/"):
        c.run("rm -rf build/")

Though yes, it falls under "trying to save users from themselves" which is less than ideal. I try to balance treating users as adults with guiding newbies (or even advanced users) away from shooting themselves in the foot, and it's not easy.