Introduction of the Concept base class #135
Conversation
| class Database(AbstractDB, metaclass=SingletonFactory): | ||
| """Class used to inject dependency on a database framework. | ||
| # pylint: disable=too-few-public-methods,abstract-method,too-many-public-methods | ||
| class Database(Wrapper, metaclass=SingletonType): |
There was a problem hiding this comment.
I would drop the abstract and make a wrapper like we do for views; automatically fetch wrapped concept's attributes if not overriden by the wrapper.
There was a problem hiding this comment.
You mean, drop the ABCMeta metaclass from AbstractDB?
| try: | ||
| Database(of_type=dbtype, **db_opts) |
There was a problem hiding this comment.
Why remove of_type? It should still work based on Dataset.__init__, right?
There was a problem hiding this comment.
Because the Database wrapper takes care of constructing the dictionary, it just removes unnecessary fluff from the call and reduces the complexity of the API, since you don't have to remember to call through explicit arguments
src/orion/core/utils/__init__.py
Outdated
| for inherited_class in cls.types: | ||
| if inherited_class.__name__.lower() == of_type.lower(): | ||
| inh_qualified_name = get_qualified_name(inherited_class.__module__, |
There was a problem hiding this comment.
inh_qualified_name 😱
inherited_class_name maybe?
corneauf
force-pushed
the
feature/concepts
branch
from
99ac76e
to
1bf37f2
Compare
Codecov Report
@@ Coverage Diff @@
## develop #135 +/- ##
==========================================
Coverage ? 92.57%
==========================================
Files ? 82
Lines ? 10604
Branches ? 718
==========================================
Hits ? 9817
Misses ? 710
Partials ? 77
Continue to review full report at Codecov.
|
|
@DatCorno I made a PR for small changes: corneauf#3 Sorry for the horribly long delay! Once this is ready for merge, I'll fix the dependency of |
|
@DatCorno Can you rebase it? It should trigger the tests at the same time. |
By using `plotter->'lib_name'->'plot_name'` inside their config files, the user specifies out of which library his specific plot needs to be created. For this purpose, the Factory metaclass has been changed to now process qualified names for types instead of just name class. This is to prevent collisions in cases like this one : `orion.viz.panda.bar` and `orion.viz.matplot.bar` which would both evaluate to `bar`
Why: Things like `BaseAlgorithm`, `BasePlotter` and the likes all shared the same code when it came to calling their factory class for dependency injection and runtime polymorphism. This changes move that code to another abstract level inside a `Concept` class. This duplicate code can then be deleted. How: The `Concept` class deduces some things about the classes inheriting it: the base class name (`BaseAlgorithm` for example), the name of the concept (`Algorithm` in that case), and the root module for the implementations of each subclass. The `Concept` then creates a factory class from the base class. This mimics the old behavior of declaring a `ThingFactory` class having `Factory` as its metaclass (like `OptimisationAlgorithm`). This change also implies that the factories' name are not available outside `Concept`. The `Factory` class has been changed as such that it'll look for the entry points identified by the `Concept` subclass rather then the `Factory` one.
Why: with the new way the Factory class handles its typenames, leaving DumbAlgo inside conftest.py lead to a lot of errors that were not warranted. By moving DumbAlgo to its own dependency like Gradient Descent, we make sure it follows the `orion.algo` pattern
Why: This was creating problems when the name in the config file would have some uppercase letters. We now work exclusively in lowercase
How: All possible concepts for testing purposes now live inside their own little egg. This allows us to test their creation (which requires a package-like hierarchy) without add test files to Orion source code
Why: the `capture=no` option inside the devel environment was creating problems with the branching functional tests since OSError was not raised.
Why: By splitting on "orion/src" the tests for wrappers were failing since they were not inside the main orion source directory. We now split on `orion` then append the `orion` string to the start of the module's name.
Why: Concepts intializing Concepts led to a lot of problems. The `Wrapper` class moves the wanted process of cascading initialization outside the `Concept` class, which helps decouple some functionality that led to paradoxial error handling.
Why: By moving `AbstractDB` as a `Concept` and using `Database` as a `Wrapper` we uniformalize the use of wrappers throughout the app.
Why: This commit is a first attempt at changing `Wrapper`s so that they type-check as the underlying type
Why: The AbstractDB already defines all required methods for wrapped database, so the wrapper can be sure those methods exists.
Why: I see no reasons the factory should be built with `globals()` as it attributes. This will likely clutter the factories attributes for no apparent reason.
Why: When accessing the attribute of the instance, it will raise an AttributeError anyway if it is missing, no need to reimplement it.
corneauf
force-pushed
the
feature/concepts
branch
from
2dca76f
to
28c712b
Compare
|
This was long overdue. I'm closing this PR in favor of #681 which is a simplified factory inspired by this PR. I am dropping the support for auto-discovery based on package files to limit maintenance efforts and because subclass() together with entry-points is enough for our needs. |
|
Glad to see this finally being closed! What a great time we had on that airplane :') |
This PR introduces a new class called
Conceptinside the utils module. It also refactors the way the Factory metaclass works and changes to code to handle this new way of doing things.FactoryThe
Factorymetaclass was working great until now. However, it had one major drawback: it could only instantiate objects from.pyfiles that reside inside the same folder as the base class. This means that the architecture of things is very limited. The newFactorywill now recursively look for.pyfiles inside the base class folder. Classes are now stored inside thetypenameslist through their qualified name, and not just their file name to prevent any conflicts from submodules that would share the same name without the same path (orion.viz.matplot.barandorion.viz.panda.barfor example).Another change that ties in with the introduction of
Conceptis that the factories are not explicitly defined inside the code. This required some changes to the entry points definition, which will now be listed through the base class name and not the factory name.ConceptThis new class comes from the desire to extract the creation process of an object like
BaseAlgorithmso that this runtime dynamic instantiation can be use towards new type without copy-pasting the code. Classes that inherit fromConceptneed to define one class-wide constant callednamewhich will be use for logging purposes. It is a human-readable string representation of the concept (AlgorithmforBaseAlgorithmfor example).They can also define other class-wide constants. The only one that currently has an impact is the
implementation_moduleone which overrides the module name use when getting the qualified name of subtypes. This is useful in the case where the base class exists in a non-__init__.pyfile. This use case comes up when creating concepts that can be extended through other packages viapip install. Since having a__init__.pyfile inside the folder would prevent the new packages from being injected, the base concept cannot be declared there.The process of instantiating a subtype has changed a little bit. Now, if the dictionary contains multiple elements, it tries to instantiate every single one of them. If it fails, it considers that item as a class attribute which calls
setattron it.Other changes
Here's a bullet list of all the others less important changes coming with this PR
Factorycall conventionPrimaryAlgohas been moved toorion.algo.basefor consistency reasonsDatabaserequired special handling for their situation, since they are not an actualConceptchild class.DumbAlgohas been moved to its own egg so that it can be instantiated through theBaseAlgorithmand not require factory hacks.capture=nostatement inside thetox:develenvironment was causing trouble. It has been removed.