-
Notifications
You must be signed in to change notification settings - Fork 47
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
Sweeper PR #214
base: main
Are you sure you want to change the base?
Sweeper PR #214
Conversation
tango/sweeps.py
Outdated
main_config = self.override_hyperparameters(combination) | ||
# TODO: need to figure where & how to store results / way to track runs | ||
with run_experiment(main_config, include_package=[self.components]) as run_dir: | ||
# TODO: fill in something here? | ||
pass |
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.
run_experiment
is just for testing. We probably want to run our sweep experiments in subprocesses so that we can execute them in parallel. You could do something like this:
main_config = self.override_hyperparameters(combination) | |
# TODO: need to figure where & how to store results / way to track runs | |
with run_experiment(main_config, include_package=[self.components]) as run_dir: | |
# TODO: fill in something here? | |
pass | |
subprocess.call(["tango", "run", self.main_config_path, "--overrides", json.dumps(overrides)]) |
Of course this doesn't make anything run in parallel, but would be a good first step.
tango/sweeps.py
Outdated
|
||
|
||
class Sweeper(Registrable): | ||
def __init__(self, main_config_path: str, sweeps_config_path: str, components: str): |
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.
The Sweeper
will also need to be aware of the target_step
that spits out metric we're trying to optimize for, and the workspace.
def __init__(self, main_config_path: str, sweeps_config_path: str, components: str): | |
def __init__(self, main_config_path: str, sweeps_config_path: str, components: str, target_step: str, workspace: Workspace): |
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.
You may want to start by implementing #142 in a separate PR
# data class that loads the parameters | ||
# TODO: unsure about how to specify a default here? | ||
@dataclass(frozen=True) | ||
class SweepConfig(Params): |
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.
class SweepConfig(Params): | |
class SweepConfig(FromParams): |
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
Line 13 in 54c4a8d
class TangoGlobalSettings(FromParams): |
tango/sweeps_test.ipynb
Outdated
@@ -0,0 +1,393 @@ | |||
{ |
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.
Can you remove this from the PR? You can tell git to ignore this file by adding it to your global git ignore
tango/sweeps.py
Outdated
main_config_path = ( | ||
"/Users/sabhyac/Desktop/sabhya/tango/test_fixtures/sweeps/basic_test/config.jsonnet" | ||
) | ||
sweeps_config_path = ( | ||
"/Users/sabhyac/Desktop/sabhya/tango/test_fixtures/sweeps/basic_test/sweeps-config.jsonnet" | ||
) | ||
components = ( | ||
"/Users/sabhyac/Desktop/sabhya/tango/test_fixtures/sweeps/basic_test/basic_arithmetic.py" | ||
) |
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.
main_config_path = ( | |
"/Users/sabhyac/Desktop/sabhya/tango/test_fixtures/sweeps/basic_test/config.jsonnet" | |
) | |
sweeps_config_path = ( | |
"/Users/sabhyac/Desktop/sabhya/tango/test_fixtures/sweeps/basic_test/sweeps-config.jsonnet" | |
) | |
components = ( | |
"/Users/sabhyac/Desktop/sabhya/tango/test_fixtures/sweeps/basic_test/basic_arithmetic.py" | |
) |
self, | ||
main_config_path: str, | ||
sweeps_config_path: str, | ||
components: str, |
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.
The Sweeper
class needs to be aware of the Workspace
being used in order to get the results of each target step, and to pass the workspace URL when it runs each experiment (with the -w
option).
self.components = components | ||
|
||
# returns all the combinations of hyperparameters in the form of a list of lists | ||
def get_combinations(self) -> 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.
For compat with older Python versions:
def get_combinations(self) -> list: | |
def get_combinations(self) -> List: |
self.components = components | ||
|
||
# returns all the combinations of hyperparameters in the form of a list of lists | ||
def get_combinations(self) -> 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.
It would probably be better if this method was a generator function so that implementations could dynamically pick the next combination based on the history so far.
# function to override all the hyperparameters in the current experiment_config | ||
def override_hyperparameters(self, experiment_tuple: tuple) -> dict: |
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.
Docstrings should be written like this:
# function to override all the hyperparameters in the current experiment_config | |
def override_hyperparameters(self, experiment_tuple: tuple) -> dict: | |
def override_hyperparameters(self, experiment_tuple: tuple) -> dict: | |
""" | |
Generates a overrides dictionary for the current experiment config. | |
""" |
sweeps_config_path: str, | ||
components: str, | ||
): | ||
super(Registrable, self).__init__() |
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.
In recent versions of Python you can just do this:
super(Registrable, self).__init__() | |
super().__init__() |
Fixes #143 #181
Changes proposed in this pull request:
Before submitting
section of the
CONTRIBUTING
docs.Writing docstrings section of the
CONTRIBUTING
docs.After submitting