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

FEDOT integration #563

Merged
merged 5 commits into from
Jul 28, 2023
Merged

FEDOT integration #563

merged 5 commits into from
Jul 28, 2023

Conversation

nicl-nno
Copy link
Contributor

Hi!

I've added integration with FEDOT AutoML framework (https://github.com/aimclub/FEDOT) for tabular tasks.
The local runs were evaluated successfully.

We also plan to add integration for time series forecasting in the next PR.

Copy link
Collaborator

@PGijsbers PGijsbers left a comment

Choose a reason for hiding this comment

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

Thank you for taking the initiative to contribute FEDOT to the automl benchmark! 🎉
First impressions are good, though I left a few minor remarks that should be easily to resolve. I will run some local tests next week as well and provided that all works as expected it looks good to me (after the suggested changes are processed) 👍

frameworks/FEDOT/exec.py Outdated Show resolved Hide resolved
frameworks/FEDOT/exec.py Outdated Show resolved Hide resolved
frameworks/FEDOT/setup.sh Show resolved Hide resolved
Copy link
Collaborator

@PGijsbers PGijsbers left a comment

Choose a reason for hiding this comment

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

Testing it myself right now and it seems to work, but code-wise there are a few things that don't work and/or could be approved. Left some more suggestions, sorry for doing this in multiple parts.

frameworks/FEDOT/setup.sh Show resolved Hide resolved
frameworks/FEDOT/setup.sh Show resolved Hide resolved
frameworks/FEDOT/__init__.py Outdated Show resolved Hide resolved
frameworks/FEDOT/__init__.py Outdated Show resolved Hide resolved
frameworks/FEDOT/exec.py Outdated Show resolved Hide resolved
frameworks/FEDOT/exec.py Outdated Show resolved Hide resolved
frameworks/FEDOT/exec.py Outdated Show resolved Hide resolved
frameworks/FEDOT/exec.py Outdated Show resolved Hide resolved
frameworks/FEDOT/exec.py Outdated Show resolved Hide resolved
frameworks/FEDOT/exec.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@PGijsbers PGijsbers left a comment

Choose a reason for hiding this comment

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

Found a nitpick, but also this problem still remains. Just ping me when you think it is ready for review again :)

frameworks/FEDOT/exec.py Outdated Show resolved Hide resolved
@nicl-nno
Copy link
Contributor Author

@PGijsbers I added the changes according to the remaining comments. Also, I rebased my branch to main.

@nicl-nno nicl-nno requested a review from PGijsbers July 26, 2023 10:39
@PGijsbers
Copy link
Collaborator

Thanks! I’ll have a look later this week

@PGijsbers
Copy link
Collaborator

That seems to do the trick. Thanks for your effort! 🎉
As an aside, I did notice some problems running the validation benchmark (python runbenchmark.py fedot validation).
For APSFailure there was a memory error (exit status 137), and for census-income it exceeded time limit. I am not sure how reproducible they are, I had a non-ideal setup (docker mode with QEMU). I don't want this to block the merge because 1) I don't know how reproducible this is and 2) it is likely not an integration error, but I did want to bring this to your attention. If you can reproduce this it is worth a look.

@PGijsbers PGijsbers merged commit 54916d6 into openml:master Jul 28, 2023
8 of 35 checks passed
@nicl-nno
Copy link
Contributor Author

Thank you, we will investigate it and try to reproduce.

Comment on lines +49 to +53
with Timer() as predict:
predictions = fedot.predict(features=dataset.test.X)
probabilities = None
if is_classification:
probabilities = fedot.predict_proba(features=dataset.test.X, probs_for_all_classes=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor comment: You should time only one of these predict calls, not both, otherwise your predict time will be twice as long as it should be.

I'd recommend copying the way it is done in the AutoGluon framework exec.py.

Copy link
Collaborator

@PGijsbers PGijsbers Jul 28, 2023

Choose a reason for hiding this comment

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

D’oh! Good catch! Opened an issue for it: #581.

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.

3 participants