Skip to content
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

Unify Graph class hierarchy #750

Merged
merged 37 commits into from
Sep 1, 2022
Merged

Unify Graph class hierarchy #750

merged 37 commits into from
Sep 1, 2022

Conversation

gkirgizov
Copy link
Collaborator

@gkirgizov gkirgizov commented Jun 28, 2022

That's an important PR that starts big changes in the core of the core: Graph hierarchy design.

My analysis of the current situation with Graphs:
There is a lot of explicit functionality that tries to achieve the following points:

  1. Draw a clear boundary between Optimiser & problem domain (AutoML/EPDE in future/etc) -- done through duplicated Graph & OptGraph classes (same for GraphNode & OptNode classes)
  2. Share single Graph implementation -- done through GraphOperator & NodeOperator classes. Graph/OptGraph just delegated these calls to Operators.

Yet this approach is not without its problems:

  • Graph & OptGraph still duplicate code, that complicates maintenance and further development of the framework core.
  • GraphOperator requires cyclic reference to the owned graph, that complicates some code (e.g. serialization).
  • Adapter functionality becomes more complex without good reason -- in effect, it just implements runtime transformations between classes where they could be avoided altogether (i.e. Pipeline is already a Graph, which is the same as OptGraph)

Same points can be achieved with a cleaner design: What previously was done manually (sharing of graph implementation between OptGraph & Graph through the GraphOperator) could be actually encoded in class hierarchy. So now instead of runtime transformations and delegation to GraphOperator everything is encoded in class hierarchy, while implementation details are hidden behind opaque Graph interface.


Additional benefits:

  • We can have more (and maybe external) Graph implementations. For example, now it's in principle possible to add network-x Graph implementation, which would give us more guarantees in correctness and efficiency of graph operations.

The continuation of this work will be #742 . Place of Adapters will be preserved and we still will be able to optimise completely foreign structures -- as far as they can be translated to Graph. But now framework users will have additional choice: if their optimised structure conforms to the Graph interface, no adaptation is required, as Optimiser will work through the Graph interface.

Specific notes on my proposed changes:

  • Graph interface is introduced
  • GraphOperator becomes the only implementation of Graph
  • GraphDelegate provides inheritance through composition -- in effect, substitutes what was done previously through delegation to self.operator
  • OptGraph now is just a wrapper that adds self.log to Graph -- could remove it after Logger as Singleton #622, and just make it an alias of GraphDelegate
  • Introduce Copyable mixin-class. GraphDelegate & GraphOperator inherit from it. (that's instead of direct impl of copy & deepcopy in OptGraph)
  • OptNode is just an alias for GraphNode
  • NodeOperator is dropped (now it's unnecessary because there's only one GraphNode impl)
  • Avoid the need for explicit passing of DEFAULT_PARAMS_STUB in OptNode and slightly refactor handling of it

Fixes #741

@gkirgizov gkirgizov self-assigned this Jun 28, 2022
@gkirgizov gkirgizov requested a review from nicl-nno June 28, 2022 13:22
@gkirgizov gkirgizov added the core Core logic related to graph optimisation label Jun 28, 2022
@gkirgizov gkirgizov requested a review from maypink June 29, 2022 16:26
NodePostprocessCallable = Callable[[Graph, Sequence[GraphNode]], Any]


class GraphOperator(Graph, Copyable):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тут стоит надо пояснить, зачем GraphOperator нужен.
И его название точно актуально? Просто ниже у тебя есть

GraphImpl = GraphOperator

что усложняет понимание разницы между GraphOperator/Graph.

Мб их надо назвать Graph/AbstactGraph?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Да, переименую перед мержем. Или маленьким отдельным PR — чтобы этот не засорять кучей мелких изменений.
GraphOperator не очень актуален, но опять же — это чтобы пока поменьше несущественных изменений видеть.

def __eq__(self, other) -> bool:
return self.operator.is_graph_equal(other)
# return self.operator.is_graph_equal(other)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

вероятно забывашка

@codecov
Copy link

codecov bot commented Aug 8, 2022

Codecov Report

Merging #750 (2b54886) into master (9a71807) will decrease coverage by 0.01%.
The diff coverage is 91.92%.

@@            Coverage Diff             @@
##           master     #750      +/-   ##
==========================================
- Coverage   87.40%   87.39%   -0.02%     
==========================================
  Files         192      193       +1     
  Lines       13299    13230      -69     
==========================================
- Hits        11624    11562      -62     
+ Misses       1675     1668       -7     
Impacted Files Coverage Δ
fedot/core/pipelines/verification_rules.py 95.05% <ø> (+0.48%) ⬆️
fedot/core/serializers/serializer.py 96.42% <ø> (ø)
fedot/core/dag/graph.py 77.21% <73.13%> (-20.01%) ⬇️
fedot/core/pipelines/pipeline.py 97.05% <87.50%> (-0.07%) ⬇️
fedot/core/dag/graph_node.py 96.96% <95.34%> (-3.04%) ⬇️
fedot/core/dag/graph_delegate.py 96.36% <96.36%> (ø)
fedot/core/dag/graph_operator.py 95.21% <97.53%> (+2.62%) ⬆️
fedot/core/composer/gp_composer/gp_composer.py 95.12% <100.00%> (ø)
fedot/core/composer/random_composer.py 100.00% <100.00%> (ø)
fedot/core/dag/node_operator.py 100.00% <100.00%> (+8.51%) ⬆️
... and 21 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@gkirgizov gkirgizov force-pushed the 608-opt branch 2 times, most recently from bd48e74 to c03ddd2 Compare August 25, 2022 09:04
@gkirgizov gkirgizov merged commit 97341b4 into master Sep 1, 2022
@gkirgizov gkirgizov deleted the 608-opt branch December 1, 2022 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core logic related to graph optimisation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Abstract Graph interface and unify Graph/OptGraph hierarchies
4 participants