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

Integration tests fix #1171

Merged
merged 10 commits into from
Sep 25, 2023
Merged

Integration tests fix #1171

merged 10 commits into from
Sep 25, 2023

Conversation

IIaKyJIuH
Copy link
Collaborator

Made all integration tests pass.

Comment on lines 103 to 105
predefined_model = Pipeline(PipelineNode('ridge', nodes_from=[PipelineNode('lagged')]))

automl.fit(path, target='sea_height', predefined_model=predefined_model)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Эти строки не нарушают контракт юнит-теста?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Нарушают смысл, потому что тут идея в том чтобы протестировать оптимизацию структуры пайплайна с использованием mock-а для удаленной вычислительной среды. А с predefined_model оптимизатор вообще не стартует.

А падал именно этот тест?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Да, модель не всегда вычисляется, зачастую при обучении выдаётся ошибка "No models were found". Но когда срабатывает, то будет пайплайн со скриншота.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm. Ну давай пока просто отключим этот тест, функциональность не критичная. Пометь плз как issue.

@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Merging #1171 (93261af) into master (17b2ecd) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1171   +/-   ##
=======================================
  Coverage   79.70%   79.70%           
=======================================
  Files         141      141           
  Lines        9851     9851           
=======================================
  Hits         7852     7852           
  Misses       1999     1999           

test/integration/remote/test_remote_composer.py Outdated Show resolved Hide resolved
@@ -76,13 +76,14 @@ def run_exogenous_experiment(path_to_file, len_forecast=250, with_exog=True, vis
task_params=task.task_params,
timeout=10,
initial_assumption=pipeline,
max_pipeline_fit_time=1,
available_operations=['lagged', 'ridge', 'exog_ts'],
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.

Сделал так, потому что без этого выбираться модели, которые не обучаются на данных теста. Например, "sparse_lagged" выбиралась и вызывала ошибку, но не только.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Стоит добавить сюда табличных моделек (хотя бы rf и knn)

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.

Вообще убрал эти правки, кажется, в новой версии master, это уже исправно работает.

@aim-pep8-bot
Copy link

aim-pep8-bot commented Sep 22, 2023

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-09-25 07:41:11 UTC

@pep8speaks
Copy link

pep8speaks commented Sep 22, 2023

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-09-22 15:13:27 UTC

predict = automl.predict(path)
shutil.rmtree(os.path.join(fedot_project_root(), 'test', 'data', 'remote', 'fitted_pipeline')) # recursive deleting
assert predict is not None
# TODO: "No models were found" error as for 22.09.2023 appears. Fix it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Вроде можно в pytest фикстурой отключать тесты

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Спасибо, не знал.
Так намного красивее.

@IIaKyJIuH IIaKyJIuH merged commit 92cee16 into master Sep 25, 2023
9 checks passed
@IIaKyJIuH IIaKyJIuH deleted the itests_fix branch September 25, 2023 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants