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

Add Hebo #915

Merged
merged 15 commits into from
May 20, 2022
Merged

Add Hebo #915

merged 15 commits into from
May 20, 2022

Conversation

lebrice
Copy link
Collaborator

@lebrice lebrice commented May 13, 2022

Migration of the HEBO algorithm from a plugin repo on GitHub into the Orion repo.

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
@lebrice lebrice requested review from bouthilx and Delaunay May 16, 2022 15:56
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
@bouthilx
Copy link
Member

LGTM. There may be 2 comments worth creating issues, the other one is only a minor fix.

@Delaunay
Copy link
Collaborator

If you move the hebo tests to the long/ folder you can add hebo to the test-long matrix CI job and it will run hebo in parallel to the unit tests. You might also increase the max-parallel job in the test-long CI so make sure all the test-long run in parallel

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
@bouthilx
Copy link
Member

HEBO tests take a significant amount of time to run, so it should be moved in long folder and run in the test-long matrix as pointed out by @Delaunay .

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
@lebrice
Copy link
Collaborator Author

lebrice commented May 18, 2022

HEBO tests take a significant amount of time to run, so it should be moved in long folder and run in the test-long matrix as pointed out by @Delaunay .

@bouthilx agreed! I moved the tests to the "long" folder, which I then renamed to "extras". (I also adjusted the workflow names to reflect this).
I suggest we place all tests related to algos that are installed through extras-require in that folder, regardless of how quick they run.

This seems good to go on my end. Lmk if you have any comments (@Delaunay as well).

@lebrice
Copy link
Collaborator Author

lebrice commented May 18, 2022

Oops, I forgot to increase the max parallel jobs value (as suggested by @Delaunay ) I'll do that now

.github/workflows/build.yml Outdated Show resolved Hide resolved
@lebrice lebrice requested a review from bouthilx May 19, 2022 15:28
@bouthilx
Copy link
Member

@bouthilx agreed! I moved the tests to the "long" folder, which I then renamed to "extras". (I also adjusted the workflow names to reflect this).
I suggest we place all tests related to algos that are installed through extras-require in that folder, regardless of how quick they run.

I disagree. The number of algorithms will continue growing and this will increase significantly the number of jobs. I believe we should try to reduce the number of jobs, and the criterion to do so is whether the algorithm takes to much time to run sequentially or not.

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
@lebrice lebrice merged commit 48de76d into develop May 20, 2022
@lebrice lebrice deleted the hebo branch May 20, 2022 17:35
@bouthilx bouthilx added the feature Introduces a new feature label May 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Introduces a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants