Integration tests fix v2 #1183
Integration tests fix v2 #1183
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1183 +/- ##
==========================================
- Coverage 79.49% 79.27% -0.22%
==========================================
Files 145 145
Lines 10022 10047 +25
==========================================
- Hits 7967 7965 -2
- Misses 2055 2082 +27 ☔ View full report in Codecov by Sentry. |
|
Hello @IIaKyJIuH! 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-21 12:27:45 UTC |
IIaKyJIuH
force-pushed
the
itests_fix_2
branch
from
85afa46 to
6ef2b94
Compare
|
Hello @IIaKyJIuH! 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-21 12:27:36 UTC |
kasyanovse
force-pushed
the
itests_fix_2
branch
from
1b804cb to
fe2391f
Compare
fedot/core/operations/evaluation/operation_implementations/models/boostings_implementations.py
Show resolved
Hide resolved
| @@ -33,15 +33,15 @@ def get_ts_data(dataset='australia', horizon: int = 30, validation_blocks=None): | |||
|
|
|||
|
|
|||
| def run_ts_forecasting_example(dataset='australia', horizon: int = 30, timeout: float = None, | |||
| visualization=False, with_tuning=True, validation_blocks=2): | |||
| visualization=False, with_tuning=False, validation_blocks=2): | |||
There was a problem hiding this comment.
Если правильно понимаю, в этом примере раньше демонстрировался и композинг, и тюнинг
Зачем отключать тюнинг?
There was a problem hiding this comment.
Тюнинг часто натыкается на несовместимые параметры у катбуста или некорректное окно у лаггед преобразования и вылетает с ошибкой. Чтобы уменьшить количество тестов, которые вылетают с этой проблемой, местами был отключен тюнинг.
There was a problem hiding this comment.
А можешь подробнее рассказать о несовместимых параметрах катбуста?
Я правильно понимаю, катбуст там может появиться после лаггед преобразования?
Почему нельзя в таком случае просто задать кастомное пространство поиска гиперпараметров или вообще убрать катбуст из списка возможных моделей?
There was a problem hiding this comment.
А можешь подробнее рассказать о несовместимых параметрах катбуста?
Например: min_data_in_leaf может быть использовано только с grow_policy in ['Lossguide', 'Depthwise']. В остальных случаях использование этого параметра выдает ошибку.
Я правильно понимаю, катбуст там может появиться после лаггед преобразования?
Да. В этом тесте проблема только с лаггед преобразованием, тут катбуст не используется в композиции.
Почему нельзя в таком случае просто задать кастомное пространство поиска гиперпараметров или вообще убрать катбуст из списка возможных моделей?
- Если задавать кастомное пространство поиска гиперпараметров:
2. то его надо будет определить для всех параметров катбуста, учтя все возможные сочетания
3. это нужно будет сделать везде, где он в теории может примениться
4. нужно будет переписать функцию мутации параметров (скорее всего) - Если вариант исключить катбуст из тестирования прокатит, то я за, но он тогда не будет протестирован)) Вообще, катбусту очень не хватает юнит тестов. Ловить всякие ошибки и опечатки на интеграционниках очень сложно.
There was a problem hiding this comment.
@kasyanovse Встречал еще подобный параметр у катбуста и добавлял в имплементацию в федоте такую проверку, чтобы она их исправляла. Может быть добавить еще одно такое правило, что если указан min_data_in_leaf и не используется нужное grow_policy, то исправлять эти параметры.
There was a problem hiding this comment.
2. Если вариант исключить катбуст из тестирования прокатит, то я за, но он тогда не будет протестирован)) Вообще, катбусту очень не хватает юнит тестов. Ловить всякие ошибки и опечатки на интеграционниках очень сложно.
Насколько я знаю, катбуст относительно недавно добавили в модели
Я однозначно за отдельный PR с юнит-тестами для него. И я однозначно за интеграционники, которые покрывают катбуст, но их можно и отдельно сделать
Поэтому я за вариант убрать катбуст из этого теста
А только в этом тесте из-за него что-то падало?
И призываю @nicl-nno. Что думаешь? Какой вариант тебе кажется лучше?
There was a problem hiding this comment.
Вообще, есть вариант просто убрать мутацию параметров в композере для тестов.
Вот совсем против такой штуки
Мутация параметров в композере - это одна из базовых операций, есть риск очень многое поломать
There was a problem hiding this comment.
Если я правильно понимаю проблему - то вот такой вариант не поможет сейчас обойти её:
Вернее graph.descriptive_id
?
There was a problem hiding this comment.
Поэтому я за вариант убрать катбуст из этого теста
Кажется, что вместе с катбустом надо будет убрать вообще все best-quality модели.
А только в этом тесте из-за него что-то падало?
Нет. У меня от 1 до 3 тестов падает с одной и той же ошибкой _catboost.CatBoostError: max_leaves option works only with lossguide tree growing. На сервере больше одного не падает.
PS. Вчера еще раз пустил тесты, появилась новая ошибка. Попробую отловить у себя.
Мутация параметров в композере - это одна из базовых операций, есть риск очень многое поломать
Как временный вариант. Однострочная правка поможет избежать изменения нескольких тестов. Или можно отключить падение композера и тюнера в тестах с созданием issue на включение после решения проблем.
There was a problem hiding this comment.
Если я правильно понимаю проблему - то вот такой вариант не поможет сейчас обойти её:
Должен сработать. Потестирую пока буду ловить новую ошибку.
This reverts commit ac4c926.
Rework of integration tests passing.