feat: enable external plugins for all CLI commands - #2061
Conversation
610aa25 to
b32c6d0
Compare
|
/ok to test b32c6d0 |
There was a problem hiding this comment.
I know it's an inherited problem, but I'd suggest to move definitions from __init__.py to a dedicated module
There was a problem hiding this comment.
this would be harder / uglier than it seems.
I can't move old things like PARENT_DIR plus I need to call _augment_sys_path in init. George suggests using component_search_roots in _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.
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__
gwarmstrong
left a comment
There was a problem hiding this comment.
I think there's a few useful command here that are missing --search-dir, e.g., all of the following I think have valid use-cases, but aren't enabled here:
gym env resolve \
--search-dir ../my-plugin \
--config configs/my-config.yaml
gym dataset render \
--search-dir ../my-plugin \
--input data.jsonl \
--prompt-config prompts/my-prompt.yaml \
--output rendered.jsonl
gym env test \
--search-dir ../my-plugin \
--resources-server my-server
gym env packages \
--search-dir ../my-plugin \
--resources-server my-server
| return roots[0] / p | ||
|
|
||
|
|
||
| def _augment_sys_path() -> None: |
There was a problem hiding this comment.
Could _augment_sys_path() use component_search_roots() in some way instead of rebuilding the root order separately? The current order looks correct, but file lookup and Python imports still get their ordering from different logic. Sharing the same function would make it less likely that they diverge later.
E.g., if we have:
/tmp/my-plugin/
└── benchmarks/
└── aime24/
├── __init__.py
├── config.yaml
└── prepare.py
When running with:
gym eval prepare --search-dir /tmp/my-plugin --benchmark aime24
Gym should load both:
/tmp/my-plugin/benchmarks/aime24/config.yaml
/tmp/my-plugin/benchmarks/aime24/prepare.py
And I think it would be easier to maintain consistency if those were all resolved in a single function, rather than separate functions.
There was a problem hiding this comment.
good point. it also made me realize that in _augment_sys_path() we don't have Path.cwd() and WORKING_DIR, while we have them in component_search_roots. also I noticed that WORKING_DIR = PARENT_DIR if _is_editable_install else Path.cwd() so this looks redundant.
I will clean this up
|
/ok to test 7db8854 |
|
…ir for gym list and search env var allows to pass extra dirs deep into the code, including rollouts colletion, prompt templates, config paths and env.yaml loading. Co-Authored-By: George Armstrong <georgea@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
…ogging too Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
…of replacing Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
…age: gym list [-h] {benchmarks,environments,agents} ...
List available components (benchmarks, agents, environments).
positional arguments:
{benchmarks,environments,agents}
benchmarks List available benchmarks.
environments List available environments by name.
agents List agent harnesses and how each composes (Pattern A
vs self-contained B).
options:
-h, --help show this help message and exit
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
…rmatting Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
… paths Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
…lugins Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
7db8854 to
504b7b2
Compare
|
/ok to test 504b7b2 |
Extended usage of
--search-dirflag and incorporates env vars logic from #1264 by @gwarmstrong