Repository navigation
feat: enable external plugins for all CLI commands #2061
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
Changes from all commits
876b943
b46b562
4974075
1bb25d7
5efc29d
a25aacb
41935a3
4534f1c
849525d
504b7b2
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 |
|---|---|---|
|
|
@@ -12,9 +12,12 @@ | |
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| import logging | ||
| import os | ||
| import sys | ||
| from os import environ | ||
| from pathlib import Path | ||
| from typing import Callable, List, Optional, Union | ||
|
|
||
|
|
||
| # /path/to/dir/Gym (PARENT_DIR) | ||
|
|
@@ -35,31 +38,90 @@ | |
| CACHE_DIR = WORKING_DIR / "cache" | ||
| RESULTS_DIR = WORKING_DIR / "results" | ||
|
|
||
| sys.path.append(str(PARENT_DIR)) | ||
|
|
||
| # Extra component/artifact search roots, `os.pathsep`-separated. The single source of extra roots — the | ||
| # CLI's `--search-dir` is folded into this var (see nemo_gym.cli.main), so flag and env var are one channel. | ||
| # Read at call time so it reflects the current environment (incl. spawned server subprocesses). | ||
| NEMO_GYM_EXTRA_ROOTS_ENV_VAR_NAME = "NEMO_GYM_EXTRA_ROOTS" | ||
|
|
||
| def _resolve_under_cwd_or_install(path) -> Path: | ||
| """Resolve a possibly-relative path for *reading* a built-in or user-supplied file. | ||
|
|
||
| Absolute paths are returned unchanged. A relative path is tried first under the current working | ||
| directory (the user's project), then under the Gym install root (``PARENT_DIR``) where built-in | ||
| assets live in both editable and wheel installs. This mirrors ``config_paths`` resolution, so a | ||
| repo-relative path like ``resources_servers/<env>/data/example.jsonl`` resolves by name from any | ||
| cwd. If neither exists the cwd candidate is returned so error messages point at the user's cwd. | ||
| def _extra_roots() -> List[Path]: | ||
| return [Path(d) for d in os.environ.get(NEMO_GYM_EXTRA_ROOTS_ENV_VAR_NAME, "").split(os.pathsep) if d] | ||
|
|
||
| Use this for read paths only — never for write targets (e.g. metrics written next to a dataset), | ||
| which must stay relative to the user's writable cwd rather than the install root. | ||
|
|
||
| def component_search_roots(*, sys_path: List[Path] | None = None) -> List[Path]: | ||
| """Ordered, de-duplicated roots to look for a Gym component/artifact under: the ``NEMO_GYM_EXTRA_ROOTS`` | ||
| roots first, then cwd and the install root (``PARENT_DIR``, the built-ins) last. | ||
| Earlier roots win on a name collision, so user components shadow built-ins. De-duplicated by resolved | ||
| path, since cwd/install root coincide in an editable checkout. The single source of truth for where Gym | ||
| looks for components — used by path resolution (:func:`_resolve_under_cwd_or_install`), module imports | ||
| (:func:`_augment_sys_path`) and the ``gym list``/``gym search`` discovery functions. | ||
| ``sys_path`` weaves existing ``sys.path`` entries in for import precedence: they slot in after the extra | ||
| roots but before cwd/built-ins. Any entry that is itself cwd or the install root is dropped so those two | ||
| always stay last — matching the file-lookup order even in a wheel install, where the install root is | ||
| already on ``sys.path`` as ``site-packages``. | ||
| """ | ||
| # cwd and the built-ins are always searched last; drop them from the woven sys.path entries so they | ||
| # can't get pinned mid-path (e.g. site-packages == PARENT_DIR in a wheel install). | ||
| trailing = [Path.cwd(), PARENT_DIR] | ||
| trailing_resolved = {root.resolve() for root in trailing} | ||
| middle = [entry for entry in (sys_path or []) if entry.resolve() not in trailing_resolved] | ||
| candidates = [*_extra_roots(), *middle, *trailing] | ||
| roots: List[Path] = [] | ||
| seen: set[Path] = set() | ||
| for root in candidates: | ||
| resolved = root.resolve() | ||
| if resolved not in seen: | ||
| seen.add(resolved) | ||
| roots.append(root) | ||
| return roots | ||
|
|
||
|
|
||
| def _resolve_under_cwd_or_install( | ||
| rel_path: Union[str, Path], *, validator: Optional[Callable[[Path], bool]] = None | ||
| ) -> Path: | ||
| """Resolve a possibly-relative path for *reading* a built-in or user-supplied file against the ordered | ||
| :func:`component_search_roots`. | ||
| Absolute paths are returned unchanged. A relative path is returned rooted at the first root where it | ||
| exists (earliest-wins: extra roots > cwd > install root), so a repo-relative path like | ||
| ``benchmarks/<name>/config.yaml`` or ``resources_servers/<env>/data/example.jsonl`` resolves by name | ||
| from any cwd or plugin root. ``validator`` overrides the default ``Path.exists`` check — pass it when a | ||
| candidate is valid only if specific markers are present (e.g. a server dir must ship | ||
| ``requirements.txt`` or ``pyproject.toml``). If nothing matches, the path under the highest-priority | ||
| root is returned so error messages point at the user's own location. | ||
| Use this for read paths only — never for write targets (e.g. metrics written next to a dataset), which | ||
| must stay relative to the user's writable cwd rather than the install root. | ||
| """ | ||
| p = Path(path) | ||
| p = Path(rel_path) | ||
| if p.is_absolute(): | ||
| return p | ||
| cwd_candidate = Path.cwd() / p | ||
| if cwd_candidate.exists(): | ||
| return cwd_candidate | ||
| install_candidate = PARENT_DIR / p | ||
| if install_candidate.exists(): | ||
| return install_candidate | ||
| return cwd_candidate | ||
| is_valid = validator if validator is not None else Path.exists | ||
| roots = component_search_roots() | ||
| for root in roots: | ||
| candidate = root / p | ||
| if is_valid(candidate): | ||
| return candidate | ||
| return roots[0] / p | ||
|
|
||
|
|
||
| def _augment_sys_path() -> None: | ||
|
Contributor
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. Could E.g., if we have: When running with: Gym should load both: And I think it would be easier to maintain consistency if those were all resolved in a single function, rather than separate functions.
Contributor
Author
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. good point. it also made me realize that in I will clean this up
Contributor
Author
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. done in 0d58030 |
||
| """Put the artifact roots on ``sys.path`` so plugin modules (e.g. a benchmark's ``prepare.py``) import. | ||
| Extra roots go to the front and cwd/``PARENT_DIR`` (the built-ins) to the end, so import precedence | ||
| matches file resolution under :func:`component_search_roots` (a plugin shadows a same-named Gym module). | ||
| Idempotent; reads ``NEMO_GYM_EXTRA_ROOTS`` at call time, so it can be re-run after ``--search-dir`` folds | ||
| roots into the env (see nemo_gym.cli.main). | ||
| """ | ||
| new_path = [str(root) for root in component_search_roots(sys_path=(Path(path) for path in sys.path))] | ||
| logging.debug(f"Replacing sys.path {sys.path} with {new_path}") | ||
| sys.path[:] = new_path | ||
|
|
||
|
|
||
| _augment_sys_path() | ||
|
|
||
|
|
||
| # TODO: Maybe eventually we want an override for OMP_NUM_THREADS ? | ||
|
|
||
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.
I know it's an inherited problem, but I'd suggest to move definitions from
__init__.pyto a dedicated moduleThere 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 would be harder / uglier than it seems.
I can't move old things like
PARENT_DIRplus I need to call_augment_sys_pathin init. George suggests usingcomponent_search_rootsin_augment_sys_path(comment) which I think makes sense.So in principle we have a circular dependency between this new dedicated module and
__init__.py. It's possible to avoid circular import error (I think), but I prefer the current solution is a lesser evil.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.
Ok, let's consider this problem separately from this MR but I strongly discourage having anything than declaration, re-imports or variable definition in
__init__