Context Managers

[pstjohn]

This is to split off discussion on context managers specifically from #303.

One option is to adapt TimeMachine from cameo directly. I would argue this adds a lot of boilerplate code to common functions, but it would definitely make porting other code from cameo more straightforward.

It seems like TimeMachine is used in cameo for track:

• Changes to reaction bounds
• Adding / removing temporary reactions
• changes to model objectives
• (We'd probably also want to track model.solution.x_dict)

Are there other uses I'm missing?

My suggestion would probably be to add a similar mechanism to cobra.Model, where model.{{ history }} (name suggestions? _context? _history?) would keep track of changes to a model's state since it entered the context. We could then try to implement property methods at as low a level as possible to update the context objects accordingly.

Here's some pseudocode for I could imagine this working:

class Model:
    def __enter__(self):
        # Create the history object, would probably want to be a full-blown class
        model._context = {}

    def __exit__(self):
        # Loop over changes, undoing each
        for rxn_id, bounds in model._context.items():
            model.reactions.get_by_id(rxn_id).bounds = bounds
        del model._context

class Reaction:
    @property
    def lower_bound(self):
        return self._lower_bound

    @lower_bound.setter
    def lower_bound(self, new_bound):
        if hasattr(self._model, '_context'):
            self._model._context[self.id] = self.bounds
        self._lower_bound = new_bound

This is super simplified, we'd definitely want to make an object that would keep track of different types of changes, the order in which they were applied, etc. But with something like this, we could initialize a context with something like

with model:
    model.reactions.my_favorite_reaction.knock_out()
    stored_solution = model.optimize()

in which everything inside the context would hopefully be undone by the __exit__ method...

---

The other option could be to create nested contexts explicitly with functions like

model.with_media, model.with_reaction_knocked_out, etc, where we could chain them together as per @cdiener 's suggestion.

[hredestig]

In your example, the TimeMachine version is

with TimeMachine() as tm:
    model.reactions.my_favorite_reaction.knock_out(time_machine=tm)
    stored_solution = model.solve()

could you elaborate why this is much more boilerplate? The methods will anyway somehow have to define how the changes are to be undone no?

[pstjohn]

Its not much more boilerplate, thats fair. I just don't necessarily like the idea of carrying the time machine around with you if you don't have to (rather than having the model take care of that). It seems like one more API to learn, another kwarg to specify for each function. In my example, functions like knock_out can go on modifying reaction.lower_bound while being totally unaware of what's going on with the context manager. model.set_media, if we make it, won't have to worry either. A 'dumb' user could do something like

with model:
    model.reaction.my_favorite_reaction.bounds = (0,0)

And not have to worry about specifying their own undo function.

We will have to define how to undo operations, that's true. But if the context manager only has to worry about a few use cases, these might get better implemented in the property methods themselves. (i.e., model.add_reactions could check for a context manager), rather than implementing do/undo functions in something like FVA that wants to add/remove reactions in a context.

[cdiener]

I think the alternative with less boiler plate would be the copy mechanism where you copy the model once and apply the changes to the copy. This is how it is done in cobrapy now and in many packages depending on cobrapy. This has the advantage that it does not require implementing undo functions for everything. A history context obviously has more detailed control since it allows reversing individual steps within the context. However, looking at cameo ProblemCache for instance it is almost never used like that. Usually the context is opened, the model modified and everything is reset. In that case a model.copy or explicitly resetting the modifications is probably a bit more pythonic ("explicit is better than implicit").

[pstjohn]

There's just not much reason for implementing a context manager if its just going to call model.copy. Then we're just doing

ko_model = model.copy()
ko_model.reactions.my_favorite_rxn.knock_out()
ko_model.optimize()

as usual, and we don't really need the context...

Whats the ProblemCache example? I would assume that the use of context managers would be to allow us to make temporary changes to a model without affecting its top-level scope.

So we could do something like this to check reaction essentiality:

for reaction in model.reactions:
    with model:
        reaction.knock_out()
        model.optimize()

[cdiener]

Exactly, model.copy does already resolve a lot of the problems already. I agree that this would not be efficient in a loop. Is that used a lot? Never seen that in cobrapy or other people's code?

If we implement context managers I would also prefer a few for the most important attributes like bounds and temporarily adding reactions.

[phantomas1234]

Copying models is too slow. That's why we started using the TimeMachine pattern in cameo. The reason that you don't see this a lot in the cobrapy codebase is that most functions operate on the solver object directly (which is even more boilerplate code). @pstjohn getting rid of additional kwargs sounds attractive. Why not something like

class Model:
    def __enter__(self):
        model.context = TimeMachine()

    def __exit__(self):
        model.context.reset()

class Reaction:
    @property
    def lower_bound(self):
        return self._lower_bound

    @lower_bound.setter
    def lower_bound(self, new_bound):
        if hasattr(self.model, 'context'):
            self.model.context(do=int,
                 undo=partial(setattr, self, "lower_bound", self.lower_bound))
        self._lower_bound = new_bound

since TimeMachine is already kind of a 'full-blown class' for this?

[pstjohn]

Yeah I like that pattern. I noticed you're putting in hooks to Reaction.lower_bound in #289, so we'll have to be careful about stacking these.

[cdiener]

Yeah, I agree. For simple setters we could get around some boilerplate code with an "undoable" decorator. Based on @phantomas1234's example:

def undoable(f):
    def wrapper(self, value):
        if hasattr(self, "context"):
             context = self.context
        elif hasattr(self, "model") and hasattr(self.model, "context"):
             context = self.model.context
        else:
              context = None

        if context:
             old = getattr(self, f.__name__)
             context(do=partial(f, self, value), undo=partial(f, self, old))
        else:
             f(self, value)

        return wrapper

This is assuming that the setter method has the same name as the property, otherwise you could make the decorator take this as argument. The result would be:

@lower_bound.setter
@undoable
def lower_bound(self, new_bound):
     self._lower_bound = new_bound

[pstjohn]

So before I do any of this it might make sense to query the team. One role context managers will likely have to play is in managing the temporary addition and removal reactions and metabolites to models. I'm thinking this would be easiest if we standardized the different ways objects combine in the base classes.

For instance, Model.py implements add_metabolites, add_reactions, remove_reactions, etc. Reactions implement add_metabolites, remove_from_model, etc.

My suggestion would be to move all these functions to a utils.addition module, where we define a number of functions like

• add_reaction_to_model,
• add_metabolite_to_model,
• add_metabolite_to_reaction,

and then have a master dictionary/class which, given two class names, returns the correct function. I'd then implement __iadd__(self, other) and __isub__ methods in cobra.Object which would check the classes of self and other and call the correct routine. My thought is that then we could context manage each of these methods, += would trigger a subsequent -=, and we wouldn't have to do this independently for each object.

I haven't thought through all the details, for instance

• would reactions need their own contexts? or could we make sure add_metabolite_to_reaction gets tracked in model._contexts?
• How do we figure out the stochiometry of add_metabolite_to_reaction?

But in general, we would keep the existing functions in place. Reaction.remove_from_model would look like

def remove_from_model(self, remove_orphans=False):
    self._model -= self
    if remove_orphans:
        ...find_orphans...
       for orphan in orphans:
           self._model -= orphan

and context management would get handled under the hood by the lower-level __isub__ methods.

[hredestig]

Still being rather new to the field - is repeated adding / removing or reactions and metabolites a common task? Could you mention a couple of important use-cases you have in mind that this will enable? Reason I'm wondering is that particularly given the interaction with the solver object I'm wondering if this will not be tricky to get right and if so, it is important that it is of high priority enough for the delay it could entail for porting cameo algorithms to cobrapy, and the move to devel-2 as mainstream. Of course, the model as context manager approach will give the user the feeling that anything that is done to the model will be rewound upon exit, but this I think anyway is an overly ambitious goal anyway. Raising the question how to make it clear what is resettable and not since the decorator is not visible.. Throwing error on non-supported object changes while in context could maybe be one approach.

I think the most logical definition of Model.add_reaction is in Model and not some other module.. The idea of having a dictionary to keep track of the add/remove attributes between model, gene, metabolite, reaction sounds creative but also a bit confusing although I think it mainly highlights the, perhaps necessary, graphy relationship between these classes.

Could we imagine a scenario where we introduce the CM a bit more stepwise? Supporting the simple attributes first and then introduce more features as we go forward?

[cdiener]

I never had any problems with the API as it is now. I understand you motivation with the -= and += but think it is not really transparent what that means. Also IDs can currently be non-unique between reactions and metabolites so it would be unclear what self.model -= duplicated_id does in that case. I personally like the idea of moving some of the add_* functions. But I completely agree that one of the major problems is getting all the solver hooks right. Maybe creating some helpers like

def remove_from_solver(metabolites=None, reactions=None):
   ...

def add_to_solver(metabolites=None, reactions=None):
   ...

would help to to the heavy lifting like connecting to optlang and manage the history (both are the undos of each other). At least that way you only call one additional function in all of the add_* and remove_* functions from Model and Reaction...

[mmundy42]

@hredestig I think repeated adding and removing of reactions and metabolites is uncommon. There certainly needs to be a way to add and remove but I don't see that as something that needs to be handled by a context manager.

@pstjohn From an object-oriented perspective it seems odd to move the actions that operate on an object to another place. I worry that in the future we'll have trouble maintaining the code if the code for an object is scattered in multiple places.

[pstjohn]

Good points, thanks for the comments! I'll offer a couple of counter-arguments.

On adding / removing reactions, you could imagine an assess_production_capability function:

for metabolite in model.metabolites:
    with model:
        sink = create_sink_rxn(metabolite)
        model.add_reaction(sink)
        model.objective = sink
        model.optimize()

For metabolites, you could imagine scanning over various P/O ratios, for instance.

On object-oriented code, I'd argue its cleaner to inherit the methods from cobra.Object and use operator overloading. You could also argue its better to keep all the addition / removal code in one place. Right now a maintainer of model.remove_reaction would have to go Model.py, realize the actual function is a class member of Reaction, and then edit that. Same with model.summary, for instance.

I'm getting a bit off-topic here, but ultimately everything in cobrapy will operate on a model or reaction. You could make similar arguments for implementing the entirety of cobra.manipulation or cobra.flux_analysis as class methods in Model.py, but things would get unwieldy fast. If cobra.core acted as more of a table of contents rather than the complete implementation, multiple pull requests might be a bit easier to manage with everything no longer targeting the same three files.

[cdiener]

I think regarding the context manager I agree that adding and removing reactions should be captured. Not so much because there are many examples but more form a usability stand point: its much easier to say that all operations are reversed than to have a list of a few that will and others that won't.

Sorry, I didn't understand your description before. You mean to add by reference and not by ID right? That makes more sense :) I think it's unusual to implement behavior of child classes in the parent class, but I don't think it's necessarily bad. Why is remove_reaction a member of Reaction? That sounds wrong and should be changed.

[pstjohn]

On remove_reaction, its in Model, but its just a thin wrapper around reaction.remove_from_model or reaction.delete. My point being that the methods are already somewhat fragmented.

[cdiener]

I agree. That is weird.

…

On Dec 2, 2016 16:14, "Peter St. John" ***@***.***> wrote: On remove_reaction, its in Model, but its just a thin wrapper around reaction.remove_from_model or reaction.delete. My point being that the methods are already somewhat fragmented. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#306 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AG0pD3-SghEmuzixQX92mzmA7Rb1JNaCks5rEJhAgaJpZM4KmR-G> .

[pstjohn]

Also, on the API: I wasn't planning to change it, this was more a suggestion on a way to standardize things at a lower level to make it easier to implement the CM undo's.

[cdiener]

Yep that sounds reasonable.

[hredestig]

#312 merged, let's open new issues and refer to this one if we want to continue the discussion