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

0.12 cannot use context for state handling #309

Closed
presidento opened this Issue Jan 19, 2016 · 28 comments

Comments

Projects
None yet
6 participants
@presidento
Contributor

presidento commented Jan 19, 2016

------------------
@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

This comment has been minimized.

Member

bitprophet commented Jan 20, 2016

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.

@bitprophet bitprophet added the Bug label Jan 20, 2016

@bitprophet bitprophet added this to the 0.12.1 milestone Jan 20, 2016

@presidento

This comment has been minimized.

Contributor

presidento commented Jan 20, 2016

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

@presidento

This comment has been minimized.

Contributor

presidento commented Jan 21, 2016

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

This comment has been minimized.

Contributor

presidento commented Jan 22, 2016

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

This comment has been minimized.

Member

bitprophet commented Jan 22, 2016

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

This comment has been minimized.

Contributor

presidento commented Jan 22, 2016

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

This comment has been minimized.

Member

bitprophet commented Jan 24, 2016

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

This comment has been minimized.

Member

bitprophet commented Jan 24, 2016

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

@bitprophet bitprophet modified the milestones: 0.12.2, 0.12.1 Feb 4, 2016

@tuukkamustonen

This comment has been minimized.

tuukkamustonen commented Feb 4, 2016

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

This comment has been minimized.

Member

bitprophet commented Feb 8, 2016

@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.

@bitprophet bitprophet modified the milestones: 0.12.2, 0.12.3 Feb 8, 2016

@chumpalump

This comment has been minimized.

chumpalump commented Feb 11, 2016

This change has bitten us as well. We've been using commands like invoke customer1 deploy restart_app. The first task sets the context for the commands that follow. Since much of the purpose of pyinvoke is to run shell commands in a pythonic way, I'm not sure it is worth guarding against ill-conceived "rm -fr". Just my two cents. On the other hand, if this change is staying, we can easily rewire our customer tasks, so no panicking here.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Feb 17, 2016

@chumpalump Thanks for the feedback!

To tl;dr my previous comment, I think part of the change was incidental to support another feature; I think I can wire things so that the old behavior comes back at least temporarily, then we can take a more reasoned approach to the mutability problem.

Keep an eye on here and I'll update when I have time to poke at my assumption.

@tuukkamustonen

This comment has been minimized.

tuukkamustonen commented Aug 2, 2016

I tried upgrading to 0.13 but had already forgotten this issue. @bitprophet What is your suggested workaround for sharing data from a pre-task to the actual task that was executed? Writing to a global variable or such sounds a bit scary.

@pwm1234

This comment has been minimized.

pwm1234 commented Aug 13, 2016

I also need to share state between tasks. As an example, I have prerequisite tasks that need to store results somewhere for the downstream task(s) to access.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Feb 15, 2017

Running into this now myself, which as always is a great sign that earlier ideas around purity need to be reconsidered. Notes below mostly for future-me in case he needs re-convincing:

I have a bunch of tasks that together setup a dev environment; they share a decorator (sort-of a pretask) that attempts to determine how much state has been set up already, by introspecting the current config. E.g. "config file found? cool, how about the cloud provider API client? ok, now how about the instance access keys?" etc, in each spot attempting to derive/load the necessary state if the answer was negative.

Part of this process involves secrets like autogenerated keys; these are stored in-memory whenever possible, and eventually end up in a secrets store (eg HashiCorp Vault, AWS KMS, etc), after which point they're loaded from that secrets store.

Other parts of the process involve the standing up of that secrets store.

Most of the time, all these steps run in one single meta-task which explicitly passes its original (CLI-derived) context into them...which acts as a de-facto shared state. First sign that that's a nice thing to have.

The more obvious second sign is when I tried to run a few chunks of this process as separate CLI tasks (i.e. I wanted to run only a subset of the entire process), because this skipped that meta-task, later tasks did not have access to the state objects from the earlier ones. So, for example, the secret keys generated in step 1 were not visible in (say) step 3, causing the state-gathering decorator to bail out early. And even if it had not, the secrets-store step would have then failed due to...not having any secrets to insert.


I'm still not convinced we want "every task always shares state" as the default (though I am becoming...more so), but it's quite clear we at least need something like invoke --shared-state task1 task2.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Feb 15, 2017

Skimming through the relevant code, I'm reminded there are as always complications:

  • With pre/post tasks (as opposed to top level tasks, i.e. inv task1 task2) they may, in some cases, truly not want to share the exact same configuration object/structure - because they may come out of separate collections, and collections supply one of the config levels.
    • Counterargument: they're being invoked attached to some other 'main' task, so arguably, they should be using that task's collection-driven config.
    • Counter-counter-argument: one usually really wants configuration to live "near" the code it impacts, so in this case, it's easy to imagine a pre-task that, say, sets up a server instance, and wants instance-creation-specific config to live in its module. But then you want to slap that pre-task on a bunch of other tasks elsewhere; replicating the instance creation settings to all those other modules would be bad.
    • Counter-counter-counter-argument (I can do this shit all day, don't even try me) - that sort of thing probably really wants to live elsewhere in the config hierarchy (e.g. project-level config files) or, alternately, just higher up in the collection tree (e.g. in one's root namespace collection, instead of in the "cloud" collection or w/e). That data's needed all over the place, so it should be accessible all over the place.
  • Actually, that's the only comment I've run into so far, so...yea. I may have successfully argued myself out of that one, as above. Will keep reading and testing.
@bitprophet

This comment has been minimized.

Member

bitprophet commented Mar 22, 2017

Found another reasonably compelling argument for fixing this in some manner - in a roundabout manner, it's desirable when tweaking how sudo prompts work. Specifically:

  • Auto-responding sudo functionality needs a password to respond with;
  • Storing that sudo password in code or other on-disk locations is a security no-no;
  • So in many situations one will want to be prompted for it at runtime & have the result cached;
  • Fabric 1 did so in a lazy fashion, which stinks for headless runs, giving rise to the abort_on_prompts setting; in v2 that should be default behavior, following the "API-first" design principle.
  • If one truly can't/won't pull the password from disk or a secrets store, then the "best" way to handle it given an always-aborts-on-prompts behavior, is to explicitly prompt up-front in userland (Fabric 1 also can do this via its -I flag, iirc).
  • I'd like to do that for Invoke/Fab 2 (right now, it still does the naive at-runtime getpass that Fab 1 did) but..."prompt for config data in task 1, then use it in tasks 2..N" rather requires...shared state between tasks!
  • The Aristocrats?

bitprophet added a commit that referenced this issue Mar 22, 2017

Remove the getpass sudo password prompting.
Users should fill it in via config for now.

Pending #309, they could also then prompt in a pre-task or setup task,
by hand, and put it in the config then.
@bitprophet

This comment has been minimized.

Member

bitprophet commented Mar 22, 2017

Also...that could be done as a generic task-decorator / pre-task sort of thing, similar to fabric 1's VERY old settings-requiring feature. Something for another ticket...

@bitprophet

This comment has been minimized.

Member

bitprophet commented Apr 12, 2017

Looking at actually doing this now, with the intent of solving the "but but, what if users need saving from themselves?!" problem later if ever.

Specifics, on top of the generic notes from the middle of this earlier comment:

  • Actual config cloning happens in Executor.config_for(call, config)
    • Which is also where per-collection config is merged in (this may be different for each task in a given session)
    • It's also where shell env vars are added to the config (because it may rely on everything up to collection-driven config)
    • So a given 'shared' config/context still needs these steps independently applied for each task or parameterization of a task, ideally in a manner that won't cause unexpected bugs...
      • Think inv task1 task2, where both tasks come from different subcollections, each with different sets of config keys; a naive shared-context setup would see task2 executing with its config containing the keys from task1's subcollection (even if, granted, any overlapping keys are overwritten by task2's collections' values)
      • Hoping the recent modifications to Config where user changes are stored independently, makes this easier - we could "swap out" the collection, etc config levels (the data that would naturally change between tasks) while preserving any changes made by user code (such as things done in 'setup' style tasks).
  • config_for() is only called by Executor.expand_calls, in a loop, for each call/task
  • expand_calls' implementation in Invoke itself is largely:
    • Generate and assign the context for each call via config_for as above
    • Expand the overall calls list with any pre- or post-tasks (recursively)
  • However Fabric 2 overrides it in a non-extending fashion, for now, in order to:
    • parameterize across the --hosts a,b,c flag values, taking each call and turning it into N calls (with its own 'cloning', of the call, leaving context/config cloning for the superclass' config_for)
      • Though I notice Call.clone does yet another config clone when making a new Context; so those configs are getting double-cloned under Fabric 2 at the moment.
    • deal with a singleton 'remainder' call (and then, same, parameterizing it if necessary)
  • That Fabric 2 implementation needs to unify with the Invoke one; i.e. it doesn't handle pre/post tasks at the moment (IIRC because the union of that with the hosts parameterization is not well-defined)
  • Executor.expand_calls is (aside from its own recursion) only called once, in Executor.execute (the main entry point).
@bitprophet

This comment has been minimized.

Member

bitprophet commented Apr 12, 2017

Flipping things around, what's the real fix here? I'd argue it's (as hinted in prev comment) simply to preserve user modifications. What this means is that as long as Executor preserves each (non-parameterized) task call's user-modifications config level, it can still make use of fresh Context and/or Config objects for each task execution:

  • Modifications made by user code in e.g. pre-tasks is preserved as a session proceeds, fulfilling this ticket's us case; and if done right this should work for pre/post tasks too;
  • Fresh Config creation means that appropriate per-collection (and thus per-task env var reads) data is refreshed and does not bleed between calls;
  • Expecting a fresh Context doesn't matter as much for Invoke, but does matter for Fabric 2 as the Context is a Connection which will differ for each host.
    • Though the story here during parameterized/concurrent execution is different anyways - even if one somehow expected serial execution to 'roll up' or aggregate data from each host at the Config level, that immediately breaks when you add parallel execution to the mix (and there are other ways to aggregate info from parameterized calls, anyways - usually via return values).

Going to take a stab at that - even if it's not 100% perfect I just want things to get back to where they were pre-0.12 in a manner that doesn't break fab -H a,b,c use case. Can hammer out details later.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Apr 13, 2017

Honestly, I'm pretty stupid (in my defense, insomnia kinda stinks), the problem remains just the order of operations; a cloned Config is not even the main issue here. Just that what we need is for each task execution to somehow yield the user-modifications layer of its context's config, so that Executor can take that and use it when creating a brand new Config (because we need a brand new config with its own distinct collection, env etc identity...we just want to slot in a non-empty modifications layer on top of that, which is not done at the moment.)

@bitprophet

This comment has been minimized.

Member

bitprophet commented Apr 13, 2017

Looking at details, now I'm torn between:

  • "new Config, where Config has mechanisms for replacing/prefilling the modifications and deletions levels" - it just feels conceptually wrong to allow for tweaking those levels, they're supposed to track what happened to that object
  • "reuse existing Config object, but rerun its .load_collection and .load_shell_env methods each time" - this is actually cleaner and more in line with new intent, and still works even if the wrapping Context changes (as in Fab 2), but...actually there is no but, unless I'm still stuck on the old "but but it should be a NEW OBJECT" way of thinking. Which I think is just wrong now.
    • As a bonus, keeping the same literal Config object means anybody being gross and making their own "unofficial" hacks to a Config object (hard, since most normal attr sets turn into config values automatically, but hey) doesn't have their clever hax (or necessary workaround...) break on them.
@bitprophet

This comment has been minimized.

Member

bitprophet commented Apr 13, 2017

That seems to work (going with single reused Config object that just has load_collection and load_shell_env rerun each time), so...that's nice.

One remaining issue I will not try to tackle for now, is the "we don't want this shared config when parameterizing". That requires more work re: Executor.execute deciding when to give the about-to-be-called task context a ref on self.config, and when to do something like self.config.clone() after all.

Imagine a mixed-mode session where one calls a few setup style serial tasks, then a parameterized task - the execution loop can't easily differentiate between the former, and the results of expanding the latter, even though right now we want to treat them differently.

Could extend Call some more with more flags/hints to Executor; could just claim this isn't an issue until one gets to truly concurrent execution (which would have to be different from the current, "expand into more serial calls" approach by its very nature); probably other options too.

For now, it's just not a huge problem so I'm punting. Will be closing this as soon as I make sure Fabric 2's still working.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Apr 13, 2017

Grumpity grump, wasn't quite that easy; the approach I implemented assumed Context can have its Config updated at runtime; this is true for Invoke (where Context is little more than a thin wrapper around Config) but not for Fabric (where Connection-subclassing-Context is much, much more, and does a lot of stuff at init time, like loading SSH configs.)

Going to try and solve this by making Connection more flexible re: ability to 'rerun' most of its init-time stuff if its Config is updated, but it's still annoying.

EDIT: that requires some gymnastics and I still feel grody, it might actually be better to try tacking parameterization data onto Call and creating the Context at runtime instead of expansion time...probably also much less work.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Apr 13, 2017

Yea that feels a lot cleaner actually, just subclassing Call and having 'make the context' a responsibility on Call objects seems to work fine.

bitprophet added a commit that referenced this issue Apr 13, 2017

bitprophet added a commit that referenced this issue Apr 13, 2017

First draft re #309
Tests pass, including new/changed ones
@dennybaa

This comment has been minimized.

dennybaa commented Apr 17, 2017

@bitprophet But actually does this work?

from invoke import task, run, call

@task
def foo(ctx):
    ctx['hello'] = 'world'

@task(foo)
def bar(ctx):
    print(ctx['hello'])

So 0.15 still the same

...
File "/usr/lib/python3.5/site-packages/invoke/config.py", line 164, in __getitem__
    return self._get(key)
  File "/usr/lib/python3.5/site-packages/invoke/config.py", line 175, in _get
    value = self._config[key]
KeyError: 'hello'

The issue is closed, probably fixed, do I misunderstand smth?

@dennybaa

This comment has been minimized.

dennybaa commented Apr 17, 2017

@bitprophet please repopen

@bitprophet

This comment has been minimized.

Member

bitprophet commented Apr 17, 2017

@dennybaa It's not released on PyPI yet! Doing that very soon though. Look for a version 0.16.0 to appear.

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