Ticket for updating our terminology is T213.

I'm still not quite understanding why the global context is needed (emphasis on global) and why this is a better solution than passing data directly to the directives. I'm game for renaming things to make them more easily understood, though.

Naming conventions aside, why wouldn't keeping check name in env_data and adding a concept of check name to the CheckDetail object work here? Where is the data in Context needed outside of the directives? Why not continue to use a dictionary for the context/env_data which is explicitly passed to the directives?

I'm not completely against the idea of using globals if there are no other reasonable choices but I don't see how they're required here or how the use of a global context variable is an improvement over what we already have. Sure, TAP export needs the check name but why isn't that part of the input data instead of semi-magically pulling the global context data into a place where I'm not sure it belongs?

I'm also not a huge fan of the additional test complexity this brings - it requires monkeypatching for anything that uses context instead of just passing in a dict with the relevant data.

So I used Context for this as well, and I was able to get rid of envdata from directives. Now they use only input_data, which I believe looks simpler and more reasonable.

So we trade explicit data passing for implicit data passing that can become difficult to trace. Sure, it makes the method calls shorter but I'm not convinced that makes for a better interface. I'm generally of the mind that explicit is better than implicit.

Once I had Context class ready, I realized it would be useful also outside TAP· generation code. In particular, all the directives needed access to the same data (workdir, job_id, check name), which we merged (without any clear distinction) into envdata and passed it to them. But these values are really very close to what "context" means - again, static immutable data.

So you've taken the stuff that was in env_data and changed that into a global context. Object names aside, how is that any less "arbitrary" than what is currently there?

The original idea was that input_data was for args passed into the runner - stuff that the user specifies at runtime. env_data was for the things needed for task execution but are not user-supplied in most cases. You're calling this context (which might be a better name for it) but I'm not seeing a whole lot of difference.

To be clear, I'm not saying that this particular use of globals is dangerous in itself. It just opens a door that I'd really rather leave shut unless there is no other reasonable option. I think it encourages less thought into good data flow and can lead to papering over structural/architectural issues.

After our epically confusing conversation on IRC yesterday, I think there are 3 items of concern. For the sake of clarity, I'm going to use env_data instead of context even though I'm fine changing the name

Separation Between env_data and input_data

There are concerns about a clear dividing line between what data goes in env_data and what goes in input_data. I'm all for making this clearer through usage and documentation. I'm also fine with changing env_data to context.

Either way, I think that a documenting the data provided to every check is a good idea. but I don't think this was the area that we disagree about though :)

checkname and CheckDetail

I think that checkname should be contained in the CheckDetail object. In my mind, the CheckDetail object is meant to collect data about the running check so that output generation is done in one place instead of each check worrying about producing valid TAP13. checkname is data about the check and I don't understand why it shouldn't be contained there.

I'd like to see functions like check.export_TAP() be as dumb as possible - construct TAP from the data contained in the CheckDetail object and no more.

To global or not to global

I dislike the use of a global context for several reasons (in order of importance):

1. it obscures data flow
   • by allowing global context, we would allow pulling in data members just about anywhere in the task execution process. Instead of explicitly passing data and knowing what is being passed into a task, it just attempts to pull whatever it needs outside of input_data from thin air. Obscuring the data flow can also make task debugging more difficult.
2. we run the risk of making non-python language support more difficult than it needs to be
   • I'm not saying that we should support all languages equally - that would be a bit on the crazy side. That being said, I do want to allow tasks to use non-python code and I do want to see all tasks treated the same, within reason. If we start using globals for context, we would have to treat task code not written in python differently by either extracting the needed information from the global context and passing them in as explicit arguments or saying "everything but python code has to take this hashmap representation of our context as an argument".
3. Task execution should be as encapsulated as possible
   • While actually separating the runner from libtaskotron would be on the stupid side of things to actually do, I think that there is value in isolating the task execution. I would like to see the task rely on its own data as much as possible and act as if calls to libtaskotron were to an external library. I don't think this level of decoupling is as important as making sure there isn't tight coupling between buildbot and libtaskotron or libtaskotron and resultsdb, I do think that keeping the interactions between tasks and libtaskotron as clean and straight-forward as sanely possible is a benefit for future library changes.
4. Unit tests will become more complicated
   • Instead of relying on passed-in data, any library function/method relying on context data will need to have the global context monkeypatched for unit tests. While that is only a few calls right now, I suspect that this number would grow if we started using a global context. Monkeypatching is a great way to stub out unwanted interactions but it's not always the ideal way to write unit tests. In general, passing data directly into the unit test is less complicated and generally makes unit tests more simple.
5. we'd introduce the risk of dreaded "how did that happen" bugs if something starts modifying the global context
   • The global config is more-or-less an immutable state read from config files. If we changed the code such that it read from file every single time it was called, the behavior would be unchanged. env_data is, by definition, almost completely runtime data. While it shouldn't change during execution, that data does affect how a task runs and we'd be opening the door for that kind of bug by using a global variable.

Long story short, I think that I understand most of the reasons why the global context is desired. I just think that any benefits it provides are not worth the costs/risks it would introduce.

1. it obscures data flow
   • by allowing global context, we would allow pulling in data members just about anywhere in the task execution process. Instead of explicitly passing data and knowing what is being passed into a task, it just attempts to pull whatever it needs outside of input_data from thin air. Obscuring the data flow can also make task debugging more difficult.

Is there really such a big difference between accessing a variable that was passed in, and variable that was imported? It's just a different way of "passing in". Of course, as long as the imported variables are read-only. Things get complicated once we make them read-write.

what is being passed into a task

I highlighted this section, because there might be some misconception here. I'll comment on it more below.

1. we run the risk of making non-python language support more difficult than it needs to be
   • I'm not saying that we should support all languages equally - that would be a bit on the crazy side. That being said, I do want to allow tasks to use non-python code and I do want to see all tasks treated the same, within reason.

In general, I'd gladly allow python tasks to be simpler and easier to write, because they can directly interface with our runner and our library.

If we start using globals for context, we would have to treat task code not written in python differently by either extracting the needed information from the global context and passing them in as explicit arguments or saying "everything but python code has to take this hashmap representation of our context as an argument".

Here I come back to the possible misconception. You talk about passing context into tasks/checks. But I didn't plan that, neither I implemented that. I assumed the context would be used in libraries which are called from the task itself. The task would not receive anything, nor use the context (at least I don't see a use case for it now).

And if the task were in a different language, it wouldn't be able to call our python libraries, and therefore there would be no need to receive and pass over the context.

So either you've got this part wrong, or I don't understand the quoted sentence. Please explain why we would need to pass context to tasks in other languages, thanks.

1. Task execution should be as encapsulated as possible
   • While actually separating the runner from libtaskotron would be on the stupid side of things to actually do, I think that there is value in isolating the task execution. I would like to see the task rely on its own data as much as possible and act as if calls to libtaskotron were to an external library. I don't think this level of decoupling is as important as making sure there isn't tight coupling between buildbot and libtaskotron or libtaskotron and resultsdb, I do think that keeping the interactions between tasks and libtaskotron as clean and straight-forward as sanely possible is a benefit for future library changes.

Actually, I think that global variable approach wins your argument here. If the libraries can use the global variable, the task is very much separated from libtaskotron - it receives some input (the input is chose to receive) and it can call some libtaskotron methods (without any special requirements). I consider that as standalone as it can get.

However, if we don't use the global variable approach, we need to pass the context to the task (an extra input argument it didn't choose to receive, it has to) and the task needs to pass the context to all libtaskotron calls. So there is a tighter coupling - the task needs to do some extra work, just for us.

1. Unit tests will become more complicated
   • Instead of relying on passed-in data, any library function/method relying on context data will need to have the global context monkeypatched for unit tests. While that is only a few calls right now, I suspect that this number would grow if we started using a global context. Monkeypatching is a great way to stub out unwanted interactions but it's not always the ideal way to write unit tests. In general, passing data directly into the unit test is less complicated and generally makes unit tests more simple.

It's a bit easier to pass it than monkeypatch it, but I don't see foresee the use cases where we would have real troubles with it. However, Josef suggested that relevant functions could support additional context= input parameter and use it with preference, for the purpose of easier unit testing.

1. we'd introduce the risk of dreaded "how did that happen" bugs if something starts modifying the global context
   • The global config is more-or-less an immutable state read from config files.

Actually I recommended adjusting config.log_dir in D129 if /var/log/taskotron/ is not writable and we want to use a temporary directory instead. Might, or might not be a good idea.

But that's a good point, where do you see differences between Context and Config? Because I don't see many of them. As currently designed, the Context should be even safer - it's not supposed to be changed at all.

If we changed the code such that it read from file every single time it was called, the behavior would be unchanged. env_data is, by definition, almost completely runtime data. While it shouldn't change during execution, that data does affect how a task runs and we'd be opening the door for that kind of bug by using a global variable.

I'm aware of that and that's why I designed Context to contain only static data, not the variables. The variables are still passed in. If our internal policy is not enough, we can disable any modifications by force in code - set up properties with getters but no setters, or similarly. But I didn't think it was necessary.

Long story short, I think that I understand most of the reasons why the global context is desired. I just think that any benefits it provides are not worth the costs/risks it would introduce.

I'm mostly interested in seeing your answer for the "misconception" described above.

As for the cost/benefit ratio, I decided to borrow a phrase :-)

In D93#14, @tflink wrote:

if Taskotron is successful, there will be far more people writing tasks than there will be writing directives.

Not just directives, we'll have much more task authors than people writing the whole tool. I'd rather optimize for them than for us, make it as simple as possible. That also applies to our test suite. There are some really complex libraries out in the world, perfectly tested, but so hard to use that nobody does. It's good to design code with the test suite in mind, but the priority should always be with the users, not the code itself.

In D130#8, @kparal wrote:

1. it obscures data flow
   • by allowing global context, we would allow pulling in data members just about anywhere in the task execution process. Instead of explicitly passing data and knowing what is being passed into a task, it just attempts to pull whatever it needs outside of input_data from thin air. Obscuring the data flow can also make task debugging more difficult.

Is there really such a big difference between accessing a variable that was passed in, and variable that was imported? It's just a different way of "passing in". Of course, as long as the imported variables are read-only. Things get complicated once we make them read-write.

It depends on the case, really. In this case, yes there is a difference. Data which is required for complete operation of a task (from prep to reporting) is never passed into the task and is pseudo-magically pulled out of the ether inside the library when it's needed. For stuff like the resultsdb or taskotron master URIs, I don't think it matters and that data shouldn't be passed into the task but for the context data we're talking about here, I think it's important to have a clear and explicit flow of data.

what is being passed into a task

I highlighted this section, because there might be some misconception here. I'll comment on it more below.

I probably could have been a bit clearer in my terminology. I attempted to frame things abstractly and I think of task execution as an encapsulated action - parameters are passed in and output (including log data and status codes) are returned.

With a global context, there is some data which is never passed into the python directive but from a wider context, that data is still passed into task execution as a whole even if it is just used in the library.

1. we run the risk of making non-python language support more difficult than it needs to be
   • I'm not saying that we should support all languages equally - that would be a bit on the crazy side. That being said, I do want to allow tasks to use non-python code and I do want to see all tasks treated the same, within reason.

In general, I'd gladly allow python tasks to be simpler and easier to write, because they can directly interface with our runner and our library.

Just because we can do something doesn't always mean that we should. One of the problems we had with AutoQA was tight coupling between components and while the exact usage here may not be a return to the kind of problems that prompted Taskotron instead of AutoQA v2.0, it starts down that road and I don't see enough benefit from this usage of a global context to start opening that door.

If we start using globals for context, we would have to treat task code not written in python differently by either extracting the needed information from the global context and passing them in as explicit arguments or saying "everything but python code has to take this hashmap representation of our context as an argument".

Here I come back to the possible misconception. You talk about passing context into tasks/checks. But I didn't plan that, neither I implemented that. I assumed the context would be used in libraries which are called from the task itself. The task would not receive anything, nor use the context (at least I don't see a use case for it now).

I think that my explanation above might clear this up.

And if the task were in a different language, it wouldn't be able to call our python libraries, and therefore there would be no need to receive and pass over the context.

So either you've got this part wrong, or I don't understand the quoted sentence. Please explain why we would need to pass context to tasks in other languages, thanks.

If we supported other languages, how else would we get the data contained in this global context into those directives? I see data like taskname and workdir as things which could/would be useful in tasks.

1. Task execution should be as encapsulated as possible
   • While actually separating the runner from libtaskotron would be on the stupid side of things to actually do, I think that there is value in isolating the task execution. I would like to see the task rely on its own data as much as possible and act as if calls to libtaskotron were to an external library. I don't think this level of decoupling is as important as making sure there isn't tight coupling between buildbot and libtaskotron or libtaskotron and resultsdb, I do think that keeping the interactions between tasks and libtaskotron as clean and straight-forward as sanely possible is a benefit for future library changes.

Actually, I think that global variable approach wins your argument here. If the libraries can use the global variable, the task is very much separated from libtaskotron - it receives some input (the input is chose to receive) and it can call some libtaskotron methods (without any special requirements). I consider that as standalone as it can get.

Standalone is not the same thing as making function/method calls shorter. It gets input from multiple places, not all of which are part of the call - without that input which needs to exist prior to a call, it wouldn't work.

Contrast that with a function/method call which only relies on the parameters it is passed - no extra setup is involved and all data needed is explicitly passed from a single location or could be loaded from defaults that are configured on a per-deployment basis. I see that as standalone because the per-execution data comes from one source and one source only with no sideloaded setup.

However, if we don't use the global variable approach, we need to pass the context to the task (an extra input argument it didn't choose to receive, it has to) and the task needs to pass the context to all libtaskotron calls. So there is a tighter coupling - the task needs to do some extra work, just for us.

If the context data is well defined (ie "all directives will be passed a context object with the following data: ..."), there is some coupling but that's unavoidable unless we want to start requiring every piece of data that could possibly be used in a directive to be passed in as an explicity stated argument.

I'm not arguing against specifying some sense of a context which can be used by directives without having to declare every piece of data as input. What I'm arguing against is multiple avenues for input data into those directives and completely different treatment for directives which interact with non-python code (for example, a perl directive or a ruby directive that call non-python code to do some processing, similar to how the python_directive works).

The big thing I'm still not understanding here is the "why". By making this change, we'd be violating some design goals of Taskotron in spirit (data boundaries and coupling), violating the "zen of python" (explicit is better than implicit) so that a couple of method calls are 10 characters shorter, we don't have to add checkname to CheckDetail objects and so that a couple values can be magically populated in library function calls. Am I just missing something here?

1. Unit tests will become more complicated
   • Instead of relying on passed-in data, any library function/method relying on context data will need to have the global context monkeypatched for unit tests. While that is only a few calls right now, I suspect that this number would grow if we started using a global context. Monkeypatching is a great way to stub out unwanted interactions but it's not always the ideal way to write unit tests. In general, passing data directly into the unit test is less complicated and generally makes unit tests more simple.

It's a bit easier to pass it than monkeypatch it, but I don't see foresee the use cases where we would have real troubles with it. However, Josef suggested that relevant functions could support additional context= input parameter and use it with preference, for the purpose of easier unit testing.

That would almost be worse in my mind - we'd be making the code more complex and adding another data path so that our tests don't have to hack at setup quite so much. If we went that route, why not always use the passed in context?

1. we'd introduce the risk of dreaded "how did that happen" bugs if something starts modifying the global context
   • The global config is more-or-less an immutable state read from config files.

Actually I recommended adjusting config.log_dir in D129 if /var/log/taskotron/ is not writable and we want to use a temporary directory instead. Might, or might not be a good idea.

But that's a good point, where do you see differences between Context and Config? Because I don't see many of them. As currently designed, the Context should be even safer - it's not supposed to be changed at all.

I don't understand how the log directory is related here. Config values should be immutable and consistent across executions.

Context data is, by definition, not consistent across executions.

If we changed the code such that it read from file every single time it was called, the behavior would be unchanged. env_data is, by definition, almost completely runtime data. While it shouldn't change during execution, that data does affect how a task runs and we'd be opening the door for that kind of bug by using a global variable.

I'm aware of that and that's why I designed Context to contain only static data, not the variables. The variables are still passed in. If our internal policy is not enough, we can disable any modifications by force in code - set up properties with getters but no setters, or similarly. But I didn't think it was necessary.

Long story short, I think that I understand most of the reasons why the global context is desired. I just think that any benefits it provides are not worth the costs/risks it would introduce.

I'm mostly interested in seeing your answer for the "misconception" described above.

As for the cost/benefit ratio, I decided to borrow a phrase :-)

In D93#14, @tflink wrote:

if Taskotron is successful, there will be far more people writing tasks than there will be writing directives.

Not just directives, we'll have much more task authors than people writing the whole tool. I'd rather optimize for them than for us, make it as simple as possible. That also applies to our test suite. There are some really complex libraries out in the world, perfectly tested, but so hard to use that nobody does. It's good to design code with the test suite in mind, but the priority should always be with the users, not the code itself.

So we're making users' lives easier by adding input sources that need to be monitored during debugging (assuming that they can be monitored), making it more difficult to use non-python languages and adding execution methods? Conveninence methods aren't the only way to make a tool more usable.

I agree that most things should be optimized for the task authors instead of Taskotron devs. However, I don't see this proposal as an overall win in that context - I think it adds as many problems as it solves and mostly shifts the complexity around instead of making it better.