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

Parallel population processing #199

Closed
wants to merge 65 commits into from
Closed

Conversation

kasyanovse
Copy link
Collaborator

@kasyanovse kasyanovse commented Sep 19, 2023

No description provided.

golem/core/constants.py Outdated Show resolved Hide resolved
golem/core/optimisers/genetic/gp_optimizer.py Outdated Show resolved Hide resolved
@valer1435
Copy link
Collaborator

И какое поведение будет у новой реализации, если n_jobs=1? Будет вызываться Parallel с n_jobs=1? Вроде был отдельный Dispatcher под это

@kasyanovse
Copy link
Collaborator Author

И какое поведение будет у новой реализации, если n_jobs=1? Будет вызываться Parallel с n_jobs=1? Вроде был отдельный Dispatcher под это

Проблемой Parallel с n_jobs=1 не является, потому что в таком случае код выполняется в том же потоке, в котором он вызывается. Проблему с Dispatcher буду фиксить если такой вариант распараллеливания зайдет.

golem/core/optimisers/genetic/gp_optimizer.py Outdated Show resolved Hide resolved
golem/core/optimisers/genetic/gp_optimizer.py Outdated Show resolved Hide resolved
golem/core/optimisers/genetic/gp_optimizer.py Outdated Show resolved Hide resolved
golem/core/optimisers/genetic/gp_optimizer.py Outdated Show resolved Hide resolved
golem/core/optimisers/genetic/gp_optimizer.py Outdated Show resolved Hide resolved
golem/core/optimisers/genetic/gp_optimizer.py Outdated Show resolved Hide resolved
golem/core/optimisers/genetic/operators/mutation.py Outdated Show resolved Hide resolved
golem/core/optimisers/genetic/gp_optimizer.py Outdated Show resolved Hide resolved
@maypink
Copy link
Collaborator

maypink commented Sep 20, 2023

Насчет ReproductionController: в моем понимании, это класс, ответственный за один эволюционный цикл. да, как ты правильно подметил, сейчас он есть только в EvoGraphOptimizer.

Но GOLEM позиционируется, как универсальное решение, поэтому инфраструктура должна быть реализована по своего рода "строительным блокам". В таком случае, ReproductionController очень удобен, так как он делает один эволюционный цикл атомарным для оптимизатора и + вынесение этой логики в отдельный класс делает EvoGraphOptimizer чище -- в нем находятся только те методы, за которые должен нести ответственность непосредственно оптимизатор.

Also, здесь Гриша писал про мотивацию делать ReproductionController

@kasyanovse
Copy link
Collaborator Author

Я не против вернуть ReproductionController. С учетом имеющихся правок такое решение приведет к почти полному дублированию функционала, используемого для набора начальной популяции. Если процесс воспроизводства поколения сейчас реализован нормально и не требует существенных корректировок, то как воссоздать ReproductionController? Да и в целом, необходимость полного дублирования говорит о том, что отдельный модуль, скорее всего, не нужен.

Also, здесь Гриша писал про мотивацию делать ReproductionController

Новый код удовлетворяет задаче, решаемой здесь, и в то же время смягчает недостатки подхода, предложенного там же. Единственное, что отладка не такая удобная, но у меня есть пара предложений как это можно решить, не теряя в производительности. Ну и оценить корректность реализации я могу лишь с натяжкой.

Небольшое резюме (субъективное):
Если вернуть ReproductionController:

  1. Плюсы
    1. Является универсальным строительным блоком
  2. Минусы
    1. Большая часть функционала (логического) дублирует аналогичный функционал в EvoGraphOptimizer (набор начального поколения)
    2. Не востребован как универсальный блок на текущий момент
    3. Логически он отнесен к operators, однако:
      1. В EvoGraphOptimizer он не сохраняется в EvoGraphOptimizer.operators (для него не обновляются requirements)
      2. Он сам использует другие operators внутри себя
      3. Его интерфейс отличается от других operators

Если не возвращать ReproductionController:

  1. Плюсы
    1. Меньше кода, меньше проблем, проще структура
    2. Меньше дублирования кода или функциональности
  2. Минусы
    1. Отсутствие универсального строительного блока

Свлю точку зрения я пояснил (надеюсь))), однако я не погружен в GOLEM, не знаю как будет оптимальней с точки зрения развития библиотеки. Сделаю как ты посчитаешь правильным)

@pep8speaks
Copy link

pep8speaks commented Sep 26, 2023

Hello @kasyanovse! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 5:1: F401 'golem.core.optimisers.genetic.operators.crossover.Crossover' imported but unused
Line 8:1: F401 'golem.core.optimisers.genetic.operators.mutation.Mutation' imported but unused

Line 99:121: E501 line too long (121 > 120 characters)

Line 176:121: E501 line too long (132 > 120 characters)

Line 3:1: F401 'itertools.chain' imported but unused
Line 8:1: F401 'golem.core.optimisers.graph.OptGraph' imported but unused
Line 162:32: E127 continuation line over-indented for visual indent
Line 163:32: E127 continuation line over-indented for visual indent
Line 223:1: E302 expected 2 blank lines, found 1
Line 228:5: F841 local variable 'checked_type' is assigned to but never used
Line 240:30: F541 f-string is missing placeholders

Line 2:1: F401 'copy.copy' imported but unused
Line 3:1: F401 'dataclasses.dataclass' imported but unused
Line 4:1: F401 'enum.Enum' imported but unused
Line 5:1: F401 'itertools.chain' imported but unused
Line 6:1: F401 'multiprocessing.managers.DictProxy' imported but unused
Line 8:1: F401 'queue.Queue' imported but unused
Line 9:1: F401 'random.sample' imported but unused
Line 10:1: F401 'typing.Union' imported but unused
Line 17:1: F401 'golem.core.optimisers.fitness.Fitness' imported but unused
Line 19:1: F401 'golem.core.optimisers.genetic.operators.crossover.CrossoverTypesEnum' imported but unused
Line 20:1: F401 'golem.core.optimisers.genetic.operators.mutation.MutationType' imported but unused
Line 25:1: F401 'golem.core.optimisers.graph.OptGraph' imported but unused
Line 33:1: E303 too many blank lines (3)
Line 79:121: E501 line too long (122 > 120 characters)
Line 170:45: F821 undefined name 'ReproducerWorkerTask'
Line 171:43: F821 undefined name 'ReproducerWorkerTask'
Line 214:27: F541 f-string is missing placeholders

Line 7:1: F401 'golem.core.optimisers.genetic.evaluation.MultiprocessingDispatcher' imported but unused

Line 38:51: E251 unexpected spaces around keyword / parameter equals
Line 38:53: E251 unexpected spaces around keyword / parameter equals
Line 40:14: E127 continuation line over-indented for visual indent
Line 56:1: E302 expected 2 blank lines, found 1
Line 157:5: F841 local variable '_individuals_output_count' is assigned to but never used
Line 170:5: E303 too many blank lines (2)
Line 207:1: F811 redefinition of unused 'test_genetic_node_with_nonfailed_task' from line 141
Line 219:5: F841 local variable 'final_tasks' is assigned to but never used

Line 11:1: F401 'golem.core.optimisers.genetic.operators.crossover.Crossover' imported but unused
Line 13:1: F401 'golem.core.optimisers.genetic.operators.mutation.Mutation' imported but unused

Comment last updated at 2023-11-16 14:27:58 UTC

@aim-pep8-bot
Copy link
Collaborator

aim-pep8-bot commented Sep 26, 2023

Hello @kasyanovse! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2023-11-24 14:59:31 UTC

golem/core/optimisers/genetic/gp_optimizer.py Outdated Show resolved Hide resolved
golem/core/optimisers/genetic/gp_optimizer.py Outdated Show resolved Hide resolved
golem/core/optimisers/genetic/gp_optimizer.py Outdated Show resolved Hide resolved
golem/core/optimisers/genetic/gp_optimizer.py Outdated Show resolved Hide resolved
@maypink
Copy link
Collaborator

maypink commented Oct 17, 2023

Я не против вернуть ReproductionController. С учетом имеющихся правок такое решение приведет к почти полному дублированию функционала, используемого для набора начальной популяции. Если процесс воспроизводства поколения сейчас реализован нормально и не требует существенных корректировок, то как воссоздать ReproductionController? Да и в целом, необходимость полного дублирования говорит о том, что отдельный модуль, скорее всего, не нужен.

Also, здесь Гриша писал про мотивацию делать ReproductionController

Новый код удовлетворяет задаче, решаемой здесь, и в то же время смягчает недостатки подхода, предложенного там же. Единственное, что отладка не такая удобная, но у меня есть пара предложений как это можно решить, не теряя в производительности. Ну и оценить корректность реализации я могу лишь с натяжкой.

Небольшое резюме (субъективное): Если вернуть ReproductionController:

  1. Плюсы

    1. Является универсальным строительным блоком
  2. Минусы

    1. Большая часть функционала (логического) дублирует аналогичный функционал в EvoGraphOptimizer (набор начального поколения)

    2. Не востребован как универсальный блок на текущий момент

    3. Логически он отнесен к operators, однако:

      1. В EvoGraphOptimizer он не сохраняется в EvoGraphOptimizer.operators (для него не обновляются requirements)
      2. Он сам использует другие operators внутри себя
      3. Его интерфейс отличается от других operators

Если не возвращать ReproductionController:

  1. Плюсы

    1. Меньше кода, меньше проблем, проще структура
    2. Меньше дублирования кода или функциональности
  2. Минусы

    1. Отсутствие универсального строительного блока

Свлю точку зрения я пояснил (надеюсь))), однако я не погружен в GOLEM, не знаю как будет оптимальней с точки зрения развития библиотеки. Сделаю как ты посчитаешь правильным)

я только за мультипроцессинг здесь, но надо понимать, насколько большой выигрыш мы заимеем, если откажемся от ООП в виде ReproductionController. Как мимнимум, предлагаю померить ускорение с изменениями в этом ПР и без, и, возможно, с последовательным распараллеливанием дважды для мутации и оценки, чтобы не терять ни в чем архитектурно. Если получится, что при однократном открывании потока мы действительно станем намного быстрее, тогда откажемся от репродукции в отдельном классе.

@kasyanovse
Copy link
Collaborator Author

kasyanovse commented Oct 17, 2023

я только за мультипроцессинг здесь, но надо понимать, насколько большой выигрыш мы заимеем, если откажемся от ООП в виде ReproductionController. Как мимнимум, предлагаю померить ускорение с изменениями в этом ПР и без, и, возможно, с последовательным распараллеливанием дважды для мутации и оценки, чтобы не терять ни в чем архитектурно. Если получится, что при однократном открывании потока мы действительно станем намного быстрее, тогда откажемся от репродукции в отдельном классе.

Само по себе двойное распараллеливание не даст сильной просадки по времени в обычном случае (недавно прочитал). joblib по умолчанию не глушит потоки после окончания расчетов, а ждет новых заданий.

Насчет возврата ReproductionController и старой структуры есть варианты:

  1. Оставить ReproductionController, сделать в нем мутации параллельно.
    Внутри ReproductionController мутации вызываются в цикле и проблема одного тупящего потока (когда последний поток что-то долго считает после того, как завершились остальные) может значительно снизить эффект от распараллеливания. Плюс к тому некоторый функционал останется нераспараллеленым.

  2. Переписать ReproductionController по типу того, что сейчас сделано в этом PR. В таком случае логика ReproductionController сократится до пары десятков строк. Насколько целесообразно оставлять класс, который будет частично дублировать функционал, реализованный в EvoGraphOptimizer._extend_population и состоять из пары десятков строк? Проблема одного тупящего потока будет в два раза актуальнее, чем при текущей реализации, так как многопоток будет разворачиваться два раза.

  3. Отказаться от ReproductionController, а его функционал частично объединить с EvoGraphOptimizer._extend_population и выделить в отдельную функцию. Что и было сделано. При этом:

    1. Упрощение
      2. На один модуль меньше, на один класс меньше.
      3. EvoGraphOptimizer._extend_population упростилась.
    2. Ускорение за счет:
      6. Объединения двух из трех вложенных циклов при мутациях в поколениях (три цикла - один на уровне ReproductionController, внутри него мутации в цикле по поколению, внутри каждой мутации еще один цикл)
      7. Распараллеливания
      8. При решении Empirical mutation probability #205 и выносе цикла из мутаций на уровень поколения, будет ускорение за счет уменьшения длительности распараллеленного функционала.

    В последнем случае речь идет не только о скорости распараллеливания, но и о том, что сама структура EvoGraphOptimizer в этом PR проще (ИМХО) и годится для решения Empirical mutation probability #205 (а это дополнительное ускорение).

golem/core/optimisers/genetic/operators/reproduction.py Outdated Show resolved Hide resolved

def try_mutation(ind: Individual,
mutation_type: Optional[MutationType] = None,
tries: int = self.parameters.max_num_of_mutation_attempts):
Copy link
Collaborator

Choose a reason for hiding this comment

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

напиши пж сразу что здесь возвращается, чтобы бы пр смотреть попроще

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Готово.

golem/core/optimisers/genetic/operators/reproduction.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Nov 8, 2023

Codecov Report

Attention: 169 lines in your changes are missing coverage. Please review.

Comparison is base (a46412c) 71.96% compared to head (8987b87) 70.13%.

Files Patch % Lines
.../core/optimisers/genetic/operators/reproduction.py 54.38% 104 Missing ⚠️
golem/core/optimisers/genetic/operators/node.py 0.00% 51 Missing ⚠️
...lem/core/optimisers/genetic/operators/crossover.py 56.25% 7 Missing ⚠️
...olem/core/optimisers/genetic/operators/mutation.py 65.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #199      +/-   ##
==========================================
- Coverage   71.96%   70.13%   -1.84%     
==========================================
  Files         136      137       +1     
  Lines        8130     8365     +235     
==========================================
+ Hits         5851     5867      +16     
- Misses       2279     2498     +219     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nicl-nno
Copy link
Collaborator

FYI:

aimclub/GEFEST#70
Вот тут этот PR обсуждался, мб что-то полезное можно сразу учесть. Не обязательно все сразу.

@kasyanovse kasyanovse mentioned this pull request Dec 7, 2023
13 tasks
@kasyanovse kasyanovse self-assigned this Dec 7, 2023
@kasyanovse kasyanovse added enhancement New feature or request research Experiments, hypothesis, research development opt core Related to core logic of optimizer labels Dec 7, 2023
@kasyanovse
Copy link
Collaborator Author

Move to #247

@kasyanovse kasyanovse closed this Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request opt core Related to core logic of optimizer research Experiments, hypothesis, research development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants