Add CUDS support to SimPhoNy

[mehdisadeghi]

In this PR we add a number of new features to SimPhoNy as part of #281 #285 #277 :

• [x] - Add CUDS class. The high level container for any CUDS component
• [x] - Modify engine base class to accept CUDS
• [x] - Add Simulation class. Think of it as a general wrapper
• [x] - Add base for engine extensions
• [x] - Load engine extenion metadata upon importing simphony
• [x] - Add query functions to get info about available engines
• [x] - Add store classes to hold information about StateData
• [x] - Add engine metadata tests
• [x] - Add simulation tests

[mehdisadeghi]

@kitchoi @SGGgarcia @roigcarlo @tuopuu Please review the latest changes to this PR.

[kitchoi]

Great work @mehdisadeghi ! Look forward to the final implementation of the ProxyStateDataStore/MemoryStateDataStore/change_datastore. The rest looks fine. I have a few minor comments above and the following:

class ABCEngineExtension(object):
    @abstractmethod
    def get_supported_engines(self):

     @abstractmethod
     def create_wrapper(self, ...):

Should they be (abstract) classmethod and staticmethod instead? In which case, https://github.com/simphony/simphony-common/blob/feature-cuds/simphony/engine/extension.py#L154:

        for name, value in inspect.getmembers(plugin, inspect.isclass):
            if not inspect.isabstract(value) and\
                    issubclass(value, ABCEngineExtension):
                # Load the corresponding engine extension
                extension = value()
                if not isinstance(extension, ABCEngineExtension):
                    raise ValueError('Expected ABCEngineExtension, got %s' %
                                     extension)
                self.add_extension(extension)

could become

        for name, extension in inspect.getmembers(plugin, inspect.isclass):
            if issubclass(extension, ABCEngineExtension) and extension is not ABCEngineExtension:
                # Load the corresponding engine extension
                self.add_extension(extension)

so you don't have to instantiate the extension.

It seems that EngineManager is meant to be used as a Singleton (correct me if this is not the case). Would it be possible to have its methods in a module instead of a class?

[mehdisadeghi]

Should they be (abstract) classmethod and staticmethod instead?

A good question indeed. This is possible of course. However, python has no builtin notion of abstract class or static method. The choice of method/class based approach vs function/module based approach to implement extensions is more or less a matter of taste. Moreover, it represents two different programming paradigms: object oriented programming vs functional programming style. In this case, I think the former lets us to build a more flexible framework. Turning those methods into functions reduces the benefits of having an extension class all together. One can also argue that, we can replace classes (as in your second question), with modules. A module is an inappropriate choice when we have classes right at hand in Python.

Would it be possible to have its (EngineManager) methods in a module instead of a class?

Please refer to the above answer.

@kitchoi thanks for your very good and detailed reviews. Feel free to continue the discussion.

[kitchoi]

@mehdisadeghi Thanks for your explanation.

As for my question about using a module instead, I was only referring to EngineManager which is used internally and with one instance. If you meant for it to be subclassed or have multiple instance of such class, my comment would be irrelevant and please ignore. I think module is essentially the implementation of a Singleton, at least in Python. So if you are certain of the Singleton design, go with the module, the code would be cleaner and easier to maintain. But at the end of the day, the implementation of EngineManager is very much internal that it is possible to modify it without upsetting the API; I am fine with it going ahead, so that we don't obstruct the next stage of development.

However, python has no builtin notion of abstract class or static method.

True and false. In Python 2, people have been making their own implementation for abstract class methods and abstract static methods, so much that there are now builtins in Python 3 :)

Turning those methods into functions reduces the benefits of having an extension class all together.

Yes I agree, and hence ABCEngineExtension should be a class as it is meant to be subclassed, I have no doubt about that. The questions for whether to restrict get_supported_engines and create_wrapper as classmethod/staticmethod stemmed from my observation of (1) how the extension is implemented in simlammps, in which create_wrapper does not touch self and get_supported_engines only uses self.create_wrapper, plus (2) the EngineManager assumes that the instantiation of the extension requires no more specification (extension = value()). So would we expect these methods to ever need to access the instance self? If not, wouldn't it be clearer to restrict them as abstract classmethod (staticmethod might be too strict, actually). Note that this detail affects the public API, so once we settle on it, we probably don't want to change it in the future.

[mehdisadeghi]

If you meant for it to be subclassed or have multiple instance of such class

Neither of these. It is supposed to be replace dynamically if necessary, and to be part of a larger framework (which does not exist yet).

module is essentially the implementation of a Singleton

This can be true. However, one might end up importing different modules or passing module instances around in order to assign another manager for handling extensions. I am more comfortable with classes (or biased toward them lets say).

there are now builtins in Python 3

Good to know that!

would we expect these methods to ever need to access the instance self

Yes. One example use case would be adding hooks before/after creating wrappers. The other one would be calling super class implementation of the method (which is empty now). [update: I think this could be possible also with abstract class methods?]

[kitchoi]

It [EngineManager] is supposed to be replace dynamically if necessary, and to be part of a larger framework (which does not exist yet). ... However, one might end up importing different modules or passing module instances around in order to assign another manager for handling extensions.

Do you mean modifying simphony.engine._ENGINE_MANAGER dynamically? Then perhaps it should be a public variable? (i.e. simphony.engine.engine_manager). If that's the case, I can see why a class would be better. This usage should be documented, though.

One example use case would be adding hooks before/after creating wrappers.

I would much prefer no side effect for a function that returns something. Why is this needed?

The other one would be calling super class implementation of the method (which is empty now).

I think you can pass type as the second argument for super.

[mehdisadeghi]

Do you mean modifying simphony.engine._ENGINE_MANAGER dynamically?

No, not at all. That variable is just a workaround for missing platform pieces. If we put a component manager somewhere in simphony, this variable would be filled internally using a service call to a service from an underlying package. [update: this variable will be replaced with corresponding service call all together]

I would much prefer no side effect for a function that returns something

I agree that side-effects are evil, and we don't want them. My main purpose here is making system more dynamic, by putting place-holders for possible future extensions in place. In this case, supporting an event-system or registering hook callables which should be called before/after wrapper creation.

[kitchoi]

Thanks @mehdisadeghi for your patience! I see lots are planned (services/hooks) and I don't yet know them, so please bear with me. Your comments help me to get a better idea on these plans.

If we put a component manager somewhere in simphony, this variable would be filled internally using a service call to a service from an underlying package. [update: this variable will be replaced with corresponding service call all together]

Without knowing what these services are about, I cannot comment further on the EngineManager class.

As for the implementation of the methods in ABCEngineExtension, I guess the problem for me is that I don't yet see why we would expect multiple instances of an EngineExtension, hence the "access to instance" question. It would be very helpful (for this PR and future ones) if I can get further insight on this. Thanks.

[mehdisadeghi]

Since this PR is growing in size, I prefer to merge this and continue working on the other issues in a separated PR. Having this PR merged, others can use the lammps-md example as a reference implementation and implement the wrapper extension part.

@kitchoi @roigcarlo @SGGgarcia @tuopuu Please feel free to comment on this.

[kitchoi]

I made a few minor comments in the tests. There is also one single, trivial flake8 error that flags the CI checks red. After that, good to merge.

[kitchoi]

Thanks!

[mehdisadeghi]

Thanks @kitchoi ! I'm adding more tests, I will correct the above issues.

[tuopuu]

Good work @mehdisadeghi . I just only found a few typos in the docstrings. Otherwise, good to merge! :smile:

[mehdisadeghi]

@kitchoi @tuopuu Thanks for the reviews! I'll merge after successful build.