-
Notifications
You must be signed in to change notification settings - Fork 48
feat: support metta://policy/<short_name_or_class_path> URIs for built-in policies #4561
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
base: main
Are you sure you want to change the base?
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
21c74a8 to
eb226cf
Compare
eb226cf to
d20f566
Compare
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".
| # Handle metta://policy/<builtin> URIs for built-in policies | ||
| if uri.startswith("metta://policy/"): | ||
| from mettagrid.policy.loader import discover_and_register_policies | ||
| from mettagrid.policy.policy_registry import get_policy_registry | ||
|
|
||
| identifier = uri[len("metta://policy/") :] | ||
| discover_and_register_policies() | ||
| registry = get_policy_registry() | ||
|
|
||
| # Check if it's a registered short name | ||
| if identifier in registry: | ||
| return PolicySpec(class_path=registry[identifier]) |
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.
Avoid shadowing metta:// policy names with built-ins
This new early return for metta://policy/<identifier> intercepts all such URIs before the MettaSchemeResolver path can resolve policies from the stats server. If a stats-server policy is named the same as a built‑in short name (e.g. random, stateless, lstm, mpt), metta://policy/random will now load the built‑in policy instead of the registered policy version, silently changing evaluation/training behavior. This is a regression from the previous flow where resolve_uri would route metta://policy/* through MettaSchemeResolver. Consider only treating built‑ins when explicitly namespaced (or after a failed stats lookup) so stats policies aren’t shadowed.
Useful? React with 👍 / 👎.
d20f566 to
24a4af1
Compare
42b3e9a to
24a4af1
Compare
ea84228 to
fe699c6
Compare
|

TL;DR
Added GitHub integration instructions and support for built-in policy URIs.
What changed?
policy_spec_from_urifunction to handlemetta://policy/<builtin>URIs for built-in policiesHow to test?
metta://policy/some_registered_policyURIs correctly resolve to the appropriate policy classdiscover_and_register_policies()Why make this change?
This change improves developer workflow by documenting GitHub integration practices and enhances the URI resolution system to support built-in policies through a convenient URI scheme. The
metta://policy/URI format provides a standardized way to reference built-in policies without needing to specify full file paths.