-
Notifications
You must be signed in to change notification settings - Fork 48
Replace number_of_vibes with explicit vibes list in change_vibe action config #4489
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
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| config_dict["obs"] = config_dict["obs"].copy() | ||
| config_dict["obs"].pop("features", None) | ||
| # Keep vibe_names in sync with number_of_vibes; favor the explicit count. | ||
| # Keep vibe_names in sync with vibes; favor the vibes list. | ||
| config_dict.pop("vibe_names", None) | ||
| change_vibe_cfg = config_dict.setdefault("actions", {}).setdefault("change_vibe", {}) | ||
| change_vibe_cfg["number_of_vibes"] = change_vibe_cfg.get("number_of_vibes") or len(VIBES) | ||
| game_config = GameConfig(**config_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.
Legacy configs with number_of_vibes now fail validation
convert_to_cpp_game_config now instantiates GameConfig directly from the supplied dict without translating the old actions.change_vibe.number_of_vibes field into the new vibes list. GameConfig inherits extra="forbid", so any previously serialized configs or checkpoints that still include number_of_vibes will raise a validation error when Simulation passes them through this path, breaking the backward compatibility the refactor was meant to preserve. Consider mapping the legacy key to vibes (or dropping it) before calling GameConfig(**config_dict) to keep older configs working.
Useful? React with 👍 / 👎.
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.
this seems fine? I dont know if we need older configs, maybe just older policies (maybe)
9db6bf4 to
a60b26a
Compare
f4f2626 to
ef5f7f2
Compare
a60b26a to
15f6923
Compare
ef5f7f2 to
0fdcc0b
Compare
relh
left a 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.
Socratic reviewing is great and I like this change also.
observations/actions/vibes could become a singular "language" that we have an interface to both encoding for the policy with a tokenizer and decoding from the policy
| change_vibe.number_of_vibes = 8 | ||
| has_gear = any(v.name == "gear" for v in change_vibe.vibes) | ||
| if not has_gear: | ||
| from mettagrid.config.vibes import VIBE_BY_NAME |
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.
this is kind of ugly can this import be at the top?
| change_vibe.number_of_vibes = 8 | ||
| has_gear = any(v.name == "gear" for v in change_vibe.vibes) | ||
| if not has_gear: | ||
| from mettagrid.config.vibes import VIBE_BY_NAME |
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.
this is kind of ugly can this import be at the top?
| - `change_vibe_neutral` | ||
|
|
||
| > **Note**: The number of available vibes is configurable via `change_vibe.number_of_vibes`. | ||
| > **Note**: The available vibes are configurable via `change_vibe.vibes`. |
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.
why is this change_vibe.vibes, maybe just vibes?
| - `change_vibe_neutral` | ||
|
|
||
| > **Note**: The number of available vibes is configurable via `change_vibe.number_of_vibes`. | ||
| > **Note**: The available vibes are configurable via `change_vibe.vibes`. |
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.
why is this change_vibe.vibes, maybe just vibes?
| config_dict["obs"] = config_dict["obs"].copy() | ||
| config_dict["obs"].pop("features", None) | ||
| # Keep vibe_names in sync with number_of_vibes; favor the explicit count. | ||
| # Keep vibe_names in sync with vibes; favor the vibes list. | ||
| config_dict.pop("vibe_names", None) | ||
| change_vibe_cfg = config_dict.setdefault("actions", {}).setdefault("change_vibe", {}) | ||
| change_vibe_cfg["number_of_vibes"] = change_vibe_cfg.get("number_of_vibes") or len(VIBES) | ||
| game_config = GameConfig(**config_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.
this seems fine? I dont know if we need older configs, maybe just older policies (maybe)
| # Enable change_vibe for GUI control, but create a policy config without it for random actions | ||
| cfg.game.actions.change_vibe.enabled = True | ||
| cfg.game.actions.change_vibe.number_of_vibes = 10 | ||
| from mettagrid.config.vibes import VIBES as ALL_VIBES |
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.
this is kind of ugly can this import be at the top?
15f6923 to
4773bb1
Compare
Merge activity
|

TL;DR
Refactored vibe configuration to use explicit vibe lists instead of numeric counts.
What changed?
change_vibe.number_of_vibestochange_vibe.vibesacross the codebaseHow to test?
Why make this change?
The previous approach of using a numeric count (
number_of_vibes) to limit available vibes was inflexible, as it only allowed using the first N vibes from the predefined list. This change enables explicit selection of which vibes should be available, allowing for more customized configurations without being constrained to the order of vibes in the default list. This is particularly useful for specialized environments that need specific vibes without including all preceding ones.