-
Notifications
You must be signed in to change notification settings - Fork 35
Fix single-machine cross-process registrations #69
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?
Changes from all commits
08a697d
2bdd1d3
6b8d480
cdd09a0
bf8731f
95afacb
792da12
fbc79d1
14ce53c
399d736
e2ebc4c
405b37d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,11 @@ | ||
| # Copyright (c) 2025 Robotics and AI Institute LLC. All rights reserved. | ||
|
|
||
| from typing import Dict, Tuple, Type | ||
| import importlib | ||
| import inspect | ||
| import json | ||
| import threading | ||
| from importlib.util import module_from_spec, spec_from_file_location | ||
| from typing import Type | ||
|
|
||
| from judo.tasks.base import Task, TaskConfig | ||
| from judo.tasks.caltech_leap_cube import CaltechLeapCube, CaltechLeapCubeConfig | ||
|
|
@@ -9,25 +14,95 @@ | |
| from judo.tasks.fr3_pick import FR3Pick, FR3PickConfig | ||
| from judo.tasks.leap_cube import LeapCube, LeapCubeConfig | ||
| from judo.tasks.leap_cube_down import LeapCubeDown, LeapCubeDownConfig | ||
| from judo.utils.registration import get_run_id, make_ephemeral_path | ||
|
|
||
| _registered_tasks: Dict[str, Tuple[Type[Task], Type[TaskConfig]]] = { | ||
| _registered_tasks: dict[str, tuple[Type[Task], Type[TaskConfig]]] = { | ||
| "cylinder_push": (CylinderPush, CylinderPushConfig), | ||
| "cartpole": (Cartpole, CartpoleConfig), | ||
| "fr3_pick": (FR3Pick, FR3PickConfig), | ||
| "leap_cube": (LeapCube, LeapCubeConfig), | ||
| "leap_cube_down": (LeapCubeDown, LeapCubeDownConfig), | ||
| "caltech_leap_cube": (CaltechLeapCube, CaltechLeapCubeConfig), | ||
| } | ||
| _builtin_names = set(_registered_tasks.keys()) | ||
|
|
||
| # set a run ID for this process that is used to persist programmatic registrations | ||
| _run_id = get_run_id() | ||
| _CUSTOM_REGISTRY_PATH = make_ephemeral_path("judo_tasks", _run_id) | ||
| _registry_lock = threading.Lock() | ||
|
|
||
| def get_registered_tasks() -> Dict[str, Tuple[Type[Task], Type[TaskConfig]]]: | ||
| """Returns a dictionary of registered tasks.""" | ||
|
|
||
| def _load_ephemeral_registry() -> None: | ||
| """On import, pull in any user-registered tasks from the temp file.""" | ||
| if not _CUSTOM_REGISTRY_PATH.is_file(): | ||
| return | ||
| try: | ||
| data = json.loads(_CUSTOM_REGISTRY_PATH.read_text()) | ||
| except Exception: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same nit re: bare exceptions. not sure this should be suppressed if it errors... |
||
| return | ||
|
|
||
| for name, info in data.items(): | ||
| # load Task class | ||
| task_mod = info.get("task_mod") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will always be |
||
| if task_mod and not task_mod.startswith("__main__"): | ||
| mod = importlib.import_module(task_mod) # package import preserves relative imports in that module | ||
| task_cls = getattr(mod, info["task_qn"]) | ||
| else: | ||
| # fallback: load by path | ||
| spec = spec_from_file_location(f"_judo_task_{name}", info["task_src"]) | ||
| assert spec and spec.loader, f"Could not load task module {info['task_src']}" | ||
| mod = module_from_spec(spec) | ||
| spec.loader.exec_module(mod) | ||
| task_cls = getattr(mod, info["task_qn"]) | ||
|
|
||
| # load Config class | ||
| cfg_mod = info.get("cfg_mod") | ||
| if cfg_mod and not cfg_mod.startswith("__main__"): | ||
| mod2 = importlib.import_module(cfg_mod) | ||
| cfg_cls = getattr(mod2, info["cfg_qn"]) | ||
| else: | ||
| spec2 = spec_from_file_location(f"_judo_cfg_{name}", info["cfg_src"]) | ||
| assert spec2 and spec2.loader, f"Could not load config module {info['cfg_src']}" | ||
| mod2 = module_from_spec(spec2) | ||
| spec2.loader.exec_module(mod2) | ||
| cfg_cls = getattr(mod2, info["cfg_qn"]) | ||
|
|
||
| _registered_tasks[name] = (task_cls, cfg_cls) | ||
|
|
||
|
|
||
| def _persist_ephemeral_registry() -> None: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: |
||
| """After any register_task(), write the current state of the registry to disk.""" | ||
| with _registry_lock: | ||
| out: dict[str, dict[str, str]] = {} | ||
| for name, (task_cls, cfg_cls) in _registered_tasks.items(): | ||
| if name in _builtin_names: | ||
| continue | ||
| task_src = inspect.getsourcefile(task_cls) | ||
| cfg_src = inspect.getsourcefile(cfg_cls) | ||
| if task_src and cfg_src: | ||
| out[name] = { | ||
| "task_src": task_src, | ||
| "task_qn": task_cls.__qualname__, | ||
| "cfg_src": cfg_src, | ||
| "cfg_qn": cfg_cls.__qualname__, | ||
| } | ||
| _CUSTOM_REGISTRY_PATH.parent.mkdir(parents=True, exist_ok=True) | ||
| _CUSTOM_REGISTRY_PATH.write_text(json.dumps(out, indent=2)) | ||
|
|
||
|
|
||
| # load any ephemeral registrations on import | ||
| _load_ephemeral_registry() | ||
|
|
||
|
|
||
| def get_registered_tasks() -> dict[str, tuple[Type[Task], Type[TaskConfig]]]: | ||
| """Get the currently registered tasks.""" | ||
| return _registered_tasks | ||
|
|
||
|
|
||
| def register_task(name: str, task_type: Type[Task], task_config_type: Type[TaskConfig]) -> None: | ||
| """Registers a new task.""" | ||
| """Register a new task with the Judo framework for this run only.""" | ||
| _registered_tasks[name] = (task_type, task_config_type) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should there be a
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alternatively, you could change the logic of |
||
| _persist_ephemeral_registry() | ||
|
|
||
|
|
||
| __all__ = [ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| # Copyright (c) 2025 Robotics and AI Institute LLC. All rights reserved. | ||
|
|
||
| import atexit | ||
| import os | ||
| import uuid | ||
| from pathlib import Path | ||
|
|
||
|
|
||
| def get_run_id(env_var: str = "JUDO_TASK_RUN_ID") -> str: | ||
| """Return a per-process run-ID and whether it was freshly created.""" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: this docstring seems stale |
||
| run_id = os.environ.get(env_var) | ||
| new_run = run_id is None | ||
| if new_run: | ||
| run_id = uuid.uuid4().hex | ||
| os.environ[env_var] = run_id | ||
| return run_id | ||
|
|
||
|
|
||
| def make_ephemeral_path(prefix: str, run_id: str, directory: str | Path = "/tmp") -> Path: | ||
| """Create / clean a temp JSON path and register its teardown.""" | ||
| directory = Path(directory) | ||
| directory.mkdir(parents=True, exist_ok=True) | ||
| path = directory / f"{prefix}_{run_id}.json" | ||
|
|
||
| # remove stale siblings | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe you should print a warning that you're doing this? I can't imagine why someone would do this, but if they happened to write an unrelated file |
||
| for old in directory.glob(f"{prefix}_*.json"): | ||
| if old.name != path.name: | ||
| try: | ||
| old.unlink() | ||
| except OSError: | ||
| pass | ||
|
|
||
| # ensure cleanup on interpreter exit | ||
| def _cleanup() -> None: | ||
| try: | ||
| path.unlink() | ||
| except FileNotFoundError: | ||
| pass | ||
|
|
||
| atexit.register(_cleanup) | ||
| return path | ||
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.
style: per the Google guide, I don't like catch-all except blocks. I also try to print or log the exception somewhere if I'm suppressing it.
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.
https://google.github.io/styleguide/pyguide.html#244-decision