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

Fixes for TunableGroups merging logic #327

Merged
merged 26 commits into from
May 3, 2023

Conversation

bpkroth
Copy link
Contributor

@bpkroth bpkroth commented May 2, 2023

Splitting more work off of #313.

In that PR we'd like to be able to make each Environment independently loadable.
To do this we need to be able to allow them to reference their Tunables.
However, other related Environments (e.g. local vs. remote), may also need to reference those same Tunables.
Moreover, we want both to obtain the same value.
To do this we would like the parent TunableGroups for the CompositeEnvironment for an Experiment to handout references rather than copies of Tunables to the child Environments.

This work adds that logic and some basic tests for it.

In essence, we make passing TunableGroups to an Environment expected, even if the first copy may be an empty collection. We consider this one the "root" or "parent" copy.
After that, any additional Tunables loaded by subsequent Environments get merged (not updated, so no overwriting) into the "parent" and the child gets a "view" of a subgroup of those.
In this way, when the Optimizer assigns new values to a Tunable, it is reflected at all levels.

Caveats:

  • Currently, the merge logic throws an error if any overlapping CovariantTunableGroups are non-equal in any way (e.g. extra param in one vs. another).

Leaving these for future work:

  • Add tests for nested CompositeEnvironments to ensure that we pass the "parent" to multiple levels deep, not just the first one.

mlos_bench/mlos_bench/tests/conftest.py Outdated Show resolved Hide resolved
mlos_bench/mlos_bench/launcher.py Show resolved Hide resolved
@bpkroth bpkroth marked this pull request as ready for review May 3, 2023 02:46
@bpkroth bpkroth requested a review from a team as a code owner May 3, 2023 02:46
bpkroth and others added 5 commits May 2, 2023 21:54
More work being split out from microsoft#313

- Handle `null` (in json, `None` in python) values for categoricals.
- Disallow duplicate values in categoricals.
- Provide type aware setters (compromise to a much much larger type aware rework for `Tunables` that isn't super beneficial).
- Slight method renaming for clarity.
- For convenience, allow accessing the `TunableGroups` by `Tunable`, not just it's name.
- Add `__contains__` method to `TunableGroups` so we can do `if tunable in tunable_group` checks
- Add a few more tests for `Tunable`, `TunableGroups`, and `CovariantTunableGroup` definitions.
  Coverage should be about 90% overall now, and 99+% on those files.
@bpkroth bpkroth enabled auto-merge (squash) May 3, 2023 03:45
@bpkroth bpkroth merged commit 01ad9bf into microsoft:main May 3, 2023
@bpkroth bpkroth deleted the tunable-groups-instance-references branch May 3, 2023 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants