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.

Sign up for GitHub

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

Jump to bottom

Add number of suggest() attempts in Algo wrapper #883

Merged
merged 9 commits into from
Apr 26, 2022
Merged

Add number of suggest() attempts in Algo wrapper #883

merged 9 commits into from
Apr 26, 2022

Conversation

lebrice
Copy link
Collaborator

@lebrice lebrice commented Apr 6, 2022

Fixes #882 (at least I think it does)

@lebrice
Add number of suggest() attempts in Algo wrapper
6d8b199 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Fix pylint issue
fc83ce3 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Copy link
Collaborator Author

lebrice commented Apr 8, 2022

Hey @bouthilx there's a single test failing: tests/functional/algos/test_algos.py::test_with_fidelity[tpe]
I'm unable to reproduce the failure on my end (with python 3.8, the test consistently passes)..

Would you mind trying it on your end, whenever you have 5 mins, and letting me know if it also works on your end?
Thanks!

@bouthilx
Copy link
Member

This test seems to be randomly failed since a few weeks. I suspect it is failing for the same unknown reason in this PR which isn't related to your modifications. It would be great if we could fix it soon thought.

bouthilx
bouthilx reviewed Apr 13, 2022
View reviewed changes
src/orion/core/worker/primary_algo.py Outdated
)
original = self.transformed_space.reverse(transformed_trial)
if original in self.registry:
max_suggest_attempts = 100
Copy link
Member

Choose a reason for hiding this comment

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

This should be configurable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As a constructor argument to the wrapper? Or parameter to suggest ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Made it an attribute in 872f094, is this sufficient, you think?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the delay. It's a bit tricky here since at the constructor level it would add an argument specific for the wrapper, should I don't think should be exposed to the users since the define the algo configuration directly, they are not even aware that there is a wrapper. Same for suggest, it should follow the same interface as the algorithms it is wrapping. Maybe your solution is the best one for now and we can reassess later on if needed.

@lebrice
Remove TODO note in test_primary_algo.py
f492ffd 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Remove note from primary_algo.py
619d6d6 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Add max_suggest_attempts as an attribute
872f094 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
skip flaky test
8e153ee 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Copy link
Collaborator Author

lebrice commented Apr 14, 2022

This test seems to be randomly failed since a few weeks. I suspect it is failing for the same unknown reason in this PR which isn't related to your modifications. It would be great if we could fix it soon thought.

Added a skip here: 8e153ee
Also created an issue about this: #887

@lebrice lebrice requested a review from bouthilx April 14, 2022 20:21
@lebrice
Fix bug in primary_algo.py
fe5d356 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@bouthilx
Copy link
Member

This test seems to be randomly failed since a few weeks. I suspect it is failing for the same unknown reason in this PR which isn't related to your modifications. It would be great if we could fix it soon thought.

Added a skip here: 8e153ee
Also created an issue about this: #887

Thanks!

@lebrice
Fix type annotation on algo wrapper suggest
9a67693 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Add tests for the new feature of the algo wrapper
bb73b82 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice lebrice requested a review from bouthilx April 21, 2022 18:43
@lebrice
Copy link
Collaborator Author

lebrice commented Apr 21, 2022 •
edited
Loading

@bouthilx the Pickleddb backward-compatibility tests seem to be failing in CI for some reason? I don't really understand why though, would you mind taking a look?

@lebrice
Copy link
Collaborator Author

lebrice commented Apr 25, 2022

@bouthilx This is ready to merge on my end, would you mind taking a quick look?

bouthilx
bouthilx approved these changes Apr 26, 2022
View reviewed changes
Copy link
Member

@bouthilx bouthilx left a comment

Choose a reason for hiding this comment

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

All good, thanks!

@lebrice lebrice merged commit e13326d into Epistimio:develop Apr 26, 2022
@bouthilx bouthilx added the enhancement Improves a feature or non-functional aspects (e.g., optimization, prettify, technical debt) label May 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bouthilx bouthilx bouthilx approved these changes

Assignees
No one assigned
Labels
enhancement Improves a feature or non-functional aspects (e.g., optimization, prettify, technical debt)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Algo wrapper may return no trials even if algo is not done
2 participants
@lebrice @bouthilx