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

Operation repo refactor #1253

Closed
wants to merge 96 commits into from
Closed

Operation repo refactor #1253

wants to merge 96 commits into from

Conversation

kasyanovse
Copy link
Collaborator

@kasyanovse kasyanovse commented Jan 19, 2024

This is a 🔨 code refactoring.

Summary

Рефактор тегов и пресетов:

  1. Разделение. Теперь пресеты никак не пересекаются с тегами и служат лишь как апи.
  2. Использование Enum везде, где только можно.
  3. Контроль совпадения тегов в репозитории и в коде.
  4. Упрощение кода, в связи с чем стало проще добавлять новые виды моделей.

Главная задача - утвердить или отклонить предлагаемые изменения. В связи с их большим количеством, предлагаю сначала сделать ревью этого PR, потом доработать в других PR, но в мастер влить одновременно все доработки.

В отдельных PR будет доработано:

  1. Пресет GPU. Сейчас не работает так как надо.
  2. Удален пресет TREE. Сейчас он в коде есть, но по факту его нет. Нужно вычищать код и тесты от него.
  3. TODO.
  4. Будут добавлены новые пресеты, которые будут выбираться автоматически при использовании PresetsEnum.AUTO (выбор моделей в зависимости от времени, выделенного на композицию, и размера данных так, чтобы в композицию не попали слишком требовательные по времени модели).
  5. Фикс тегов atomized_model. Сейчас это просто костыль.

После завершения этого цикла PR добавлять новые типы моделей и теги станет гораздо проще. До этого добавление неминуемо вело к усложнению и еще большему запутыванию системы пресетов/тегов.

# Conflicts:
#	fedot/core/repository/data/data_operation_repository.json
#	fedot/core/repository/operation_types_repository.py
@kasyanovse kasyanovse self-assigned this Jan 19, 2024
@pep8speaks
Copy link

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

Line 43:1: W293 blank line contains whitespace
Line 47:1: W293 blank line contains whitespace
Line 52:1: W293 blank line contains whitespace
Line 83:1: W293 blank line contains whitespace
Line 113:1: W293 blank line contains whitespace

Line 9:121: E501 line too long (143 > 120 characters)
Line 76:121: E501 line too long (133 > 120 characters)

Line 3:121: E501 line too long (124 > 120 characters)
Line 27:121: E501 line too long (132 > 120 characters)

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

Line 8:1: F401 'fedot.core.repository.operation_tags_n_repo_enums.DataOperationTagsEnum' imported but unused

Line 9:1: E305 expected 2 blank lines after class or function definition, found 1
Line 10:121: E501 line too long (123 > 120 characters)
Line 11:121: E501 line too long (127 > 120 characters)
Line 12:121: E501 line too long (127 > 120 characters)
Line 13:121: E501 line too long (133 > 120 characters)
Line 14:121: E501 line too long (137 > 120 characters)
Line 16:121: E501 line too long (127 > 120 characters)
Line 20:121: E501 line too long (136 > 120 characters)

Line 13:1: F401 'fedot.core.repository.tasks.Task' imported but unused
Line 15:1: E302 expected 2 blank lines, found 1
Line 45:65: F821 undefined name 'EvaluationStrategy'
Line 71:1: W293 blank line contains whitespace
Line 74:1: W293 blank line contains whitespace
Line 75:32: E251 unexpected spaces around keyword / parameter equals
Line 75:34: E251 unexpected spaces around keyword / parameter equals
Line 77:1: W293 blank line contains whitespace
Line 91:1: W293 blank line contains whitespace
Line 95:1: W293 blank line contains whitespace
Line 105:1: W293 blank line contains whitespace
Line 113:1: W293 blank line contains whitespace
Line 116:1: W293 blank line contains whitespace
Line 120:1: W293 blank line contains whitespace
Line 127:9: E117 over-indented
Line 130:1: W293 blank line contains whitespace
Line 174:1: W293 blank line contains whitespace
Line 178:24: E127 continuation line over-indented for visual indent
Line 179:24: E127 continuation line over-indented for visual indent

Line 1:1: F401 'enum.Enum' imported but unused
Line 1:1: F401 'enum.IntEnum' imported but unused
Line 2:1: F401 'itertools.chain' imported but unused
Line 3:1: F401 'json' imported but unused
Line 4:1: F401 'collections.defaultdict' imported but unused
Line 5:1: F401 'dataclasses.dataclass' imported but unused
Line 6:1: F401 'pathlib.Path' imported but unused
Line 7:1: F401 'typing.Union' imported but unused
Line 10:1: F401 'fedot.core.repository.operation_tags_n_repo_enums.ALL_TAGS' imported but unused
Line 10:1: F401 'fedot.core.repository.operation_tags_n_repo_enums.DataOperationTagsEnum' imported but unused
Line 10:1: F401 'fedot.core.repository.operation_tags_n_repo_enums.ModelTagsEnum' imported but unused
Line 10:121: E501 line too long (136 > 120 characters)
Line 16:1: F401 'fedot.core.repository.json_evaluation.import_enums_from_str' imported but unused
Line 16:1: F401 'fedot.core.repository.json_evaluation.import_strategy_from_str' imported but unused
Line 16:1: F401 'fedot.core.repository.json_evaluation.read_field' imported but unused
Line 20:5: F401 'fedot.core.operations.evaluation.evaluation_interfaces.EvaluationStrategy' imported but unused
Line 33:1: W293 blank line contains whitespace
Line 40:1: W293 blank line contains whitespace
Line 44:1: W293 blank line contains whitespace
Line 102:1: W293 blank line contains whitespace
Line 104:121: E501 line too long (125 > 120 characters)

Line 7:1: F401 'fedot.core.repository.operation_types_repo_enum.OperationReposEnum' imported but unused

Line 23:42: F821 undefined name 'Pipeline'

Copy link
Contributor

github-actions bot commented Jan 19, 2024

Code in this pull request still contains PEP8 errors, please write the /fix-pep8 command in the comments below to create commit with automatic fixes.

Comment last updated at

@kasyanovse kasyanovse marked this pull request as ready for review February 10, 2024 10:19
Copy link
Collaborator

@Lopa10ko Lopa10ko left a comment

Choose a reason for hiding this comment

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

занести все тэги в Enum - очень хорошая идея.
выглядит чуть более формально, чем словари в переменных класса)

в скором времени все TODO и заделы на будущее можно растащить по отдельным issues

@@ -97,7 +97,7 @@ FEDOT предоставляет высокоуровневый API, котор

.. code-block:: python

model = Fedot(problem='classification', timeout=5, preset='best_quality', n_jobs=-1)
model = Fedot(problem='classification', timeout=5, preset=PresetsEnum.BEST_QUALITY, n_jobs=-1)
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.

А один красивый импорт из fedot?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Чем проще - тем лучше. PresetsEnum.BEST_QUALITY со стороны внешнего пользователя выглядит как что-то сложное, в python-библиотеках такие решения во внешних API - большая редкость.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Есть вариант вынести константу вида BEST_QUALITY для импорта из федота. Будет что-то типа уровней логирования в logging, заданных константами на верхнем уровне библиотеки.

fedot/api/api_utils/presets.py Outdated Show resolved Hide resolved
Comment on lines +44 to +45
if value is PresetsEnum.AUTO:
value = PresetsEnum.FAST_TRAIN
Copy link
Collaborator

@Lopa10ko Lopa10ko Feb 21, 2024

Choose a reason for hiding this comment

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

я не уловил идею, почему при присваивании preset_name значение AUTO происходит подмена сразу на FAST_TRAIN? видимо, здесь пока WIP, потому что в таком случае хочется использовать модели, не требующие много времени на обучение

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Потому что пресет авто должен в какой-то момент подменяться другим пресетом или должен везде интерпретироваться как другой пресет.
Когда появится логика, опирающаяся на этот пресет, можно будет оставить его как самостоятельную единицу, но сейчас это просто синоним фаст трейна.

kasyanovse and others added 2 commits March 8, 2024 12:50
Co-authored-by: George Lopatenko <81328772+Lopa10ko@users.noreply.github.com>
@nicl-nno
Copy link
Collaborator

nicl-nno commented May 7, 2024

Closed as obsolete

@nicl-nno nicl-nno closed this May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants