-
Notifications
You must be signed in to change notification settings - Fork 1
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
Simplifies isolation mechanisms #224
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these MlFlow observer changes new? Why do they pop up under the problem registration refactor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They were actually supposed to take place in the observer refactoring, but we forgot.
pyproject.toml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 Seems like a reasonable exclusion list.
src/poli/objective_factory.py
Outdated
quiet=quiet, | ||
**kwargs_for_factory, | ||
) | ||
# if not force_isolation: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a point in cleaning up this block? Or is this temporary?
src/poli/objective_factory.py
Outdated
# evaluation_budget=evaluation_budget, | ||
# quiet=quiet, | ||
# **kwargs_for_factory, | ||
# ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
End of comment block.
# it might be because you forgot to register your problem | ||
# factory and black box inside the __init__.py file of | ||
# the objective_repository. Remember that you need to add | ||
# them to AVAILABLE_PROBLEM_FACTORIES and AVAILABLE_BLACK_BOXES. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Helpful comment:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did this one go? Or do we not want the basic examples anymore?
src/poli/core/registry.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested the logic of these changes (yet), I'm only acknowledging that I am aware of the changes to the registry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See minor comments.
The updated changes are indeed better digestible.
This PR removes
register_problem
and everything associated with it.(Closes #211 )