-
Notifications
You must be signed in to change notification settings - Fork 13
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
API refactoring + API tests and checks #33
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.
Just left some minor comments, don't need to act on them really. I do like the idea that you test runnable examples through pytest
, I think going forward this will make it easier to do integration tests and make test writing easier for people.
If you are happy with others using the examples, you could move them to the root, ./examples
directory of the repo and still test them there. That way people will see them, use them, and their tested.
src/neps/api.py
Outdated
@@ -106,7 +108,7 @@ def run( | |||
ignore_errors: bool = False, | |||
loss_value_on_error: None | float = None, | |||
cost_value_on_error: None | float = None, | |||
pre_load_hooks: List=[], | |||
pre_load_hooks: List = [], |
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.
Some information about typing, don't need to do anything here but thought it mgiht be good to share:
- If you have
from __future__ import annotations
(which there is), you can use the lowercaselist
- Do not use mutable arguments as a default.
- We can type it better by following down where they're used, from what I gather it accepts
Callable[[metahyper.Sampler], metahyper.Sampler]
, where it can modify the sampler. Consider the typing can happen at aneps
level and we only care aboutclass BaseOptimizer(metahyper.Sampler): ...
, then the full signature might look like:
def run(
...,
pre_load_hooks: list[Callable[[BaseOptimizer], BaseOptimizer]] | None = None
):
if pre_load_hooks is None:
pre_load_hooks = []
- We can actually improve it even further using
typing.Iterable
to mean anything that we can iterate
from typing import Iterable
def run(
...,
pre_load_hooks: Iterable[Callable[[BaseOptimizer], BaseOptimizer]] | None = None
):
if pre_load_hooks is None:
pre_load_hooks = []
This means someone could pass a tuple
, set
, generator
or really anything where we can go for x in my_iterable: ...
.
- Last bit is definitely over the top and low priority, but just to share some new typing tips. The function is actually a bit more specific in that it should return the same type of
BaseOptimizer
that went into it, not just any base optimizer. We can useTypeVar
's for this, which are essentially C+++ Templates or Java Generics.
from typing import TypeVar
# This is some type that must be subclass of BaseOptimizer
OptimizerT = TypeVar("OptimizerT", bound=BaseOptimizer)
def run(
...,
pre_load_hooks: Iterable[Callable[[OptimizerT], OptimizerT]] | None = None
):
if pre_load_hooks is None:
pre_load_hooks = []
@Neeratyoy thought you might also like to know about these things if you are reviewing
This PR focuses on NePS API refactoring and introduces several enhancements and tests:
neps.run
call type, covering Baseoptimizer, default YAML, and user YAML scenarios.None
to be passed for the pipeline space when the searcher is of typeBaseOptimizer
. Raises an error otherwise