Add nemo-skills-core subpackage for lightweight installs #1229
Conversation
gwarmstrong
force-pushed
the
georgea/refactor-separable-pipeline
branch
from
8fa5c7d to
4e2fad9
Compare
gwarmstrong
changed the title
gwarmstrong
force-pushed
the
georgea/refactor-separable-pipeline
branch
from
d22246e to
76c2a18
Compare
Signed-off-by: George Armstrong <[email protected]>
gwarmstrong
changed the title
gwarmstrong
force-pushed
the
georgea/refactor-separable-pipeline
branch
from
a2751f3 to
f0eb8d0
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| _EVALUATOR_MAP_PATHS[eval_type] = None | ||
| _resolved_evaluator_map[eval_type] = eval_fn |
There was a problem hiding this comment.
Setting _EVALUATOR_MAP_PATHS[eval_type] = None creates a fragile state. If _resolved_evaluator_map is ever cleared or doesn't contain the eval_type, _get_evaluator_fn will call _resolve(None) and crash.
| _EVALUATOR_MAP_PATHS[eval_type] = None | |
| _resolved_evaluator_map[eval_type] = eval_fn | |
| # Store function directly, bypassing the lazy resolution path | |
| _resolved_evaluator_map[eval_type] = eval_fn |
📝 WalkthroughWalkthroughAdds a lightweight core package and docs, reorganizes optional dependencies, introduces CI step for UV, implements lazy evaluator resolution, refactors dataset loading to prefer local modules and delegates cluster handling to a new pipeline dataset module, and adds a runtime guard for pipeline imports. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Core as nemo_skills.dataset.utils
participant Pipeline as nemo_skills.pipeline.dataset
participant Cluster
rect rgba(100, 150, 200, 0.5)
Note over User,Core: Local-only flow (default)
User->>Core: get_dataset_module(dataset, data_dir=None)
Core->>Core: import from nemo_skills.dataset or local path
Core-->>User: return dataset module
end
rect rgba(200, 100, 150, 0.5)
Note over User,Cluster: Cluster flow (deprecated in Core)
User->>Core: get_dataset_module(dataset, cluster_config=...)
Core->>Core: emit DeprecationWarning
Core->>Pipeline: delegate get_dataset_module(...)
Pipeline->>Cluster: fetch / download cluster module (remote)
Cluster-->>Pipeline: module content / init.py
Pipeline->>Core: imported module
Core-->>User: return dataset module
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@nemo_skills/evaluation/evaluator/__init__.py`:
- Around line 113-117: The error message incorrectly labels
`_EVALUATOR_MAP_PATHS.keys()` as "All supported types" when it only contains
function-based evaluators; update the ValueError text in the raise block (the
code that references eval_type) to clearly distinguish class-based vs
function-based types by either listing both maps together (combine
`_EVALUATOR_CLASS_MAP_PATHS.keys()` and `_EVALUATOR_MAP_PATHS.keys()`) or
renaming the second label to "Function-based evaluator types" so users see
accurate descriptions of `_EVALUATOR_CLASS_MAP_PATHS` and
`_EVALUATOR_MAP_PATHS`.
- Around line 106-107: register_evaluator currently stores None into
_EVALUATOR_MAP_PATHS[eval_type], which will cause AttributeError when code later
iterates or calls _resolve expecting a path string; change register_evaluator so
it stores a sentinel string (e.g. "<dynamic>") into
_EVALUATOR_MAP_PATHS[eval_type] instead of None, and ensure any
resolution/display logic in _resolve/_get_evaluator_fn treats that sentinel as a
dynamic entry (or filters it out) so rsplit is only called on real path strings;
update references to _EVALUATOR_MAP_PATHS, register_evaluator,
_resolved_evaluator_map, _get_evaluator_fn, and _resolve accordingly.
- Line 137: Remove the leftover debug print statement print(f"evaluator:
{evaluator}") from the module (it should not be in production code); either
delete that line or replace it with an appropriate logger.debug call using the
module logger (e.g., logger.debug("evaluator: %s", evaluator)) so diagnostics
use the configured logging system and not stdout—locate the print by searching
for the exact string and update in the __init__ module where the evaluator
variable is in scope.
- Around line 93-94: Remove the debug print statement print(f"evaluator:
{evaluator}") from the module so it no longer emits debug output; locate the
temporary print in the evaluator initialization block near where EVALUATOR_MAP
and EVALUATOR_CLASS_MAP are set and delete that single line, leaving the maps
and the helper functions (_get_evaluator_fn, _get_evaluator_cls, evaluate,
get_evaluator_class) intact so iteration via EVALUATOR_MAP/EVALUATOR_CLASS_MAP
still works per the documented design.
In `@nemo_skills/pipeline/dataset.py`:
- Around line 60-62: The check uses cluster_config.get("executor") which masks a
missing-key error; change it to access the key directly
(cluster_config["executor"]) so missing executor raises immediately, and keep
the logic that if cluster_config is None or cluster_config["executor"] in (None,
"none") then return _get_local_dataset_module(dataset, data_dir); update any
related code paths that assume executor exists (e.g., the code around
get_unmounted_path in nemo_skills/pipeline/utils/mounts.py) to rely on the same
direct-access semantics to fail fast on misconfiguration.
🧹 Nitpick comments (6)
CONTRIBUTING.md (1)
56-59: Fenced code block missing language specifier.Minor nit from markdownlint — adding a language (e.g.,
text) would silence MD040.Proposed fix
-``` +```text Pipeline can import from Core. Core CANNOT import from Pipeline. -``` +```core/requirements.txt (1)
17-27: Section label "math evaluation" is misleading — several packages below it aren't math-specific.
mcp,numpy,openai,requests,rich,tqdm, andtransformersare general-purpose dependencies, not math evaluation specific. Consider either reorganizing sections or using a broader label like# --- general / shared ---.nemo_skills/pipeline/dataset.py (3)
39-51: Imported module outlives its backing file.
import_from_pathis called inside aTemporaryDirectorycontext manager. Once thewithblock exits, the downloadedinit.pyis deleted, but the module object (and its__file__attribute) still references the now-removed path. This works at runtime because CPython caches the compiled bytecode in memory, but it can cause confusing errors if any downstream code inspectsmodule.__file__or attempts a reload.Consider moving the temp directory lifecycle to the caller or keeping it alive longer if module introspection is needed.
44-50: Chain the re-raised exception for clearer tracebacks.Per the static analysis hint (B904),
raise ... from errpreserves the original traceback context.Proposed fix
try: cluster_download_file(cluster_config, cluster_dataset_path, tmp_path) - except FileNotFoundError: - raise RuntimeError( + except FileNotFoundError as err: + raise RuntimeError( f"Init file {mounted_path} not found on the cluster. " f"Please check the dataset name you're using. Did you forget to run prepare data commands?" - ) + ) from err
109-113: Chain the re-raisedRuntimeErrorfor clearer tracebacks.Same B904 pattern — add
from errto preserve the originalModuleNotFoundErrorcontext.Proposed fix
- except ModuleNotFoundError: - raise RuntimeError( + except ModuleNotFoundError as err: + raise RuntimeError( f"Dataset {dataset} not found in any of the searched locations: " f"{data_dir if data_dir else 'nemo_skills.dataset'}, {extra_datasets}" - ) + ) from errnemo_skills/dataset/utils.py (1)
116-135: Chain re-raised exceptions for clearer tracebacks.Same pattern as flagged in
pipeline/dataset.py— theraise RuntimeError(...)statements at Lines 120 and 126 insideexceptclauses should usefromto preserve the original exception context.Proposed fix
except ModuleNotFoundError: dataset = dataset.replace(".", "/") extra_datasets = extra_datasets or os.environ.get("NEMO_SKILLS_EXTRA_DATASETS") if extra_datasets is None: - raise RuntimeError(f"Dataset {dataset} not found in {data_dir if data_dir else 'nemo_skills.dataset'}") + raise RuntimeError( + f"Dataset {dataset} not found in {data_dir if data_dir else 'nemo_skills.dataset'}" + ) from None if extra_datasets_type == ExtraDatasetType.local or extra_datasets_type is None: with add_to_path(extra_datasets): try: dataset_module = importlib.import_module(dataset) - except ModuleNotFoundError: - raise RuntimeError( + except ModuleNotFoundError as err: + raise RuntimeError( f"Dataset {dataset} not found in any of the searched locations: " f"{data_dir if data_dir else 'nemo_skills.dataset'}, {extra_datasets}" - ) + ) from err
| EVALUATOR_MAP = _EVALUATOR_MAP_PATHS | ||
| EVALUATOR_CLASS_MAP = _EVALUATOR_CLASS_MAP_PATHS |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check for direct bracket access to EVALUATOR_MAP and EVALUATOR_CLASS_MAP
rg -n 'EVALUATOR_MAP\[' --type=py
rg -n 'EVALUATOR_CLASS_MAP\[' --type=pyRepository: NVIDIA-NeMo/Skills
Length of output: 44
🏁 Script executed:
# Check for .values() iteration
rg -n 'EVALUATOR_MAP\.values\(\)' --type=py
rg -n 'EVALUATOR_CLASS_MAP\.values\(\)' --type=pyRepository: NVIDIA-NeMo/Skills
Length of output: 44
🏁 Script executed:
# Check for direct assignment or import of these maps
rg -n 'from.*EVALUATOR_MAP|from.*EVALUATOR_CLASS_MAP|import.*EVALUATOR_MAP|import.*EVALUATOR_CLASS_MAP' --type=pyRepository: NVIDIA-NeMo/Skills
Length of output: 44
🏁 Script executed:
# Let's examine the __init__.py file to understand the current structure
cat -n nemo_skills/evaluation/evaluator/__init__.py | head -150Repository: NVIDIA-NeMo/Skills
Length of output: 7640
🏁 Script executed:
# Check if __all__ is defined in the file
rg -n '__all__' nemo_skills/evaluation/evaluator/__init__.pyRepository: NVIDIA-NeMo/Skills
Length of output: 44
🏁 Script executed:
# Check test files that might import or use these maps
fd -e py -path '*test*' | xargs rg -l 'EVALUATOR_MAP|EVALUATOR_CLASS_MAP' 2>/dev/null || trueRepository: NVIDIA-NeMo/Skills
Length of output: 292
🏁 Script executed:
# Search for any imports of these specific exports
rg -n 'from nemo_skills.evaluation.evaluator import.*EVALUATOR_MAP|from nemo_skills.evaluation.evaluator import.*EVALUATOR_CLASS_MAP' --type=pyRepository: NVIDIA-NeMo/Skills
Length of output: 44
Remove debug print statement on line 137.
The print(f"evaluator: {evaluator}") should be removed.
Regarding the breaking change concern: The design explicitly intends EVALUATOR_MAP and EVALUATOR_CLASS_MAP to be used for iteration only (e.g., listing available types), with all actual access going through the helper functions (_get_evaluator_fn(), _get_evaluator_cls(), evaluate(), get_evaluator_class()). The comment on lines 90–92 documents this. No code in the repository accesses these maps via bracket notation, so there is no breaking change in practice.
🤖 Prompt for AI Agents
In `@nemo_skills/evaluation/evaluator/__init__.py` around lines 93 - 94, Remove
the debug print statement print(f"evaluator: {evaluator}") from the module so it
no longer emits debug output; locate the temporary print in the evaluator
initialization block near where EVALUATOR_MAP and EVALUATOR_CLASS_MAP are set
and delete that single line, leaving the maps and the helper functions
(_get_evaluator_fn, _get_evaluator_cls, evaluate, get_evaluator_class) intact so
iteration via EVALUATOR_MAP/EVALUATOR_CLASS_MAP still works per the documented
design.
| _EVALUATOR_MAP_PATHS[eval_type] = None | ||
| _resolved_evaluator_map[eval_type] = eval_fn |
There was a problem hiding this comment.
register_evaluator stores None in the path map, which will break _resolve.
When a dynamically registered evaluator is later resolved via _get_evaluator_fn, it correctly hits the _resolved_evaluator_map cache (Line 77). However, if any code iterates _EVALUATOR_MAP_PATHS values and tries to resolve them (or displays them), the None entry will cause an AttributeError on rsplit. Storing a sentinel like "<dynamic>" or filtering None values in error messages would be safer.
Proposed fix
- _EVALUATOR_MAP_PATHS[eval_type] = None
+ _EVALUATOR_MAP_PATHS[eval_type] = "<dynamically-registered>"
_resolved_evaluator_map[eval_type] = eval_fn📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _EVALUATOR_MAP_PATHS[eval_type] = None | |
| _resolved_evaluator_map[eval_type] = eval_fn | |
| _EVALUATOR_MAP_PATHS[eval_type] = "<dynamically-registered>" | |
| _resolved_evaluator_map[eval_type] = eval_fn |
🤖 Prompt for AI Agents
In `@nemo_skills/evaluation/evaluator/__init__.py` around lines 106 - 107,
register_evaluator currently stores None into _EVALUATOR_MAP_PATHS[eval_type],
which will cause AttributeError when code later iterates or calls _resolve
expecting a path string; change register_evaluator so it stores a sentinel
string (e.g. "<dynamic>") into _EVALUATOR_MAP_PATHS[eval_type] instead of None,
and ensure any resolution/display logic in _resolve/_get_evaluator_fn treats
that sentinel as a dynamic entry (or filters it out) so rsplit is only called on
real path strings; update references to _EVALUATOR_MAP_PATHS,
register_evaluator, _resolved_evaluator_map, _get_evaluator_fn, and _resolve
accordingly.
| raise ValueError( | ||
| f"Evaluator class not found for type: {eval_type}.\n" | ||
| f"Available types with class support: {list(EVALUATOR_CLASS_MAP.keys())}\n" | ||
| f"All supported types: {list(EVALUATOR_MAP.keys())}" | ||
| f"Available types with class support: {list(_EVALUATOR_CLASS_MAP_PATHS.keys())}\n" | ||
| f"All supported types: {list(_EVALUATOR_MAP_PATHS.keys())}" | ||
| ) |
There was a problem hiding this comment.
Misleading "All supported types" label — only shows function-based types.
Line 116 lists _EVALUATOR_MAP_PATHS.keys() under the label "All supported types", but it only includes function-based evaluators, not class-based ones. This is confusing for users hitting this error.
Proposed fix
raise ValueError(
f"Evaluator class not found for type: {eval_type}.\n"
f"Available types with class support: {list(_EVALUATOR_CLASS_MAP_PATHS.keys())}\n"
- f"All supported types: {list(_EVALUATOR_MAP_PATHS.keys())}"
+ f"All supported types: {sorted(list(_EVALUATOR_CLASS_MAP_PATHS.keys()) + list(_EVALUATOR_MAP_PATHS.keys()))}"
)🧰 Tools
🪛 Ruff (0.15.0)
[warning] 113-117: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In `@nemo_skills/evaluation/evaluator/__init__.py` around lines 113 - 117, The
error message incorrectly labels `_EVALUATOR_MAP_PATHS.keys()` as "All supported
types" when it only contains function-based evaluators; update the ValueError
text in the raise block (the code that references eval_type) to clearly
distinguish class-based vs function-based types by either listing both maps
together (combine `_EVALUATOR_CLASS_MAP_PATHS.keys()` and
`_EVALUATOR_MAP_PATHS.keys()`) or renaming the second label to "Function-based
evaluator types" so users see accurate descriptions of
`_EVALUATOR_CLASS_MAP_PATHS` and `_EVALUATOR_MAP_PATHS`.
| if eval_type in EVALUATOR_CLASS_MAP: | ||
| if eval_type in _EVALUATOR_CLASS_MAP_PATHS: | ||
| evaluator = get_evaluator_class(eval_type, eval_config) | ||
| print(f"evaluator: {evaluator}") |
There was a problem hiding this comment.
Debug print left in production code.
This looks like a leftover debug statement.
Proposed fix
- print(f"evaluator: {evaluator}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| print(f"evaluator: {evaluator}") |
🤖 Prompt for AI Agents
In `@nemo_skills/evaluation/evaluator/__init__.py` at line 137, Remove the
leftover debug print statement print(f"evaluator: {evaluator}") from the module
(it should not be in production code); either delete that line or replace it
with an appropriate logger.debug call using the module logger (e.g.,
logger.debug("evaluator: %s", evaluator)) so diagnostics use the configured
logging system and not stdout—locate the print by searching for the exact string
and update in the __init__ module where the evaluator variable is in scope.
| if cluster_config is None or cluster_config.get("executor") in (None, "none"): | ||
| # Delegate to core for local-only loading | ||
| return _get_local_dataset_module(dataset, data_dir) |
There was a problem hiding this comment.
Use direct dictionary access for executor instead of .get().
cluster_config["executor"] is accessed directly elsewhere (e.g., Line 70, and in get_unmounted_path from nemo_skills/pipeline/utils/mounts.py), so executor is expected to be present. Using .get() here silently treats a missing key the same as None, masking a configuration error.
Proposed fix
- if cluster_config is None or cluster_config.get("executor") in (None, "none"):
+ if cluster_config is None or cluster_config["executor"] in (None, "none"):As per coding guidelines, "Do not use .get() for accessing dictionary keys if the code expects them to be present; use direct dictionary access dict[key] instead to allow proper error handling and fail fast with clear errors".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if cluster_config is None or cluster_config.get("executor") in (None, "none"): | |
| # Delegate to core for local-only loading | |
| return _get_local_dataset_module(dataset, data_dir) | |
| if cluster_config is None or cluster_config["executor"] in (None, "none"): | |
| # Delegate to core for local-only loading | |
| return _get_local_dataset_module(dataset, data_dir) |
🤖 Prompt for AI Agents
In `@nemo_skills/pipeline/dataset.py` around lines 60 - 62, The check uses
cluster_config.get("executor") which masks a missing-key error; change it to
access the key directly (cluster_config["executor"]) so missing executor raises
immediately, and keep the logic that if cluster_config is None or
cluster_config["executor"] in (None, "none") then return
_get_local_dataset_module(dataset, data_dir); update any related code paths that
assume executor exists (e.g., the code around get_unmounted_path in
nemo_skills/pipeline/utils/mounts.py) to rely on the same direct-access
semantics to fail fast on misconfiguration.
| _EVALUATOR_MAP_PATHS[eval_type] = None | ||
| _resolved_evaluator_map[eval_type] = eval_fn |
There was a problem hiding this comment.
Setting _EVALUATOR_MAP_PATHS[eval_type] = None is fragile. If _resolved_evaluator_map gets cleared or doesn't contain eval_type, _get_evaluator_fn will call _resolve(None) and crash with ValueError: not enough values to unpack.
The current implementation works only because the function is immediately added to _resolved_evaluator_map, but this implicit dependency is error-prone. Consider either:
- Not setting
_EVALUATOR_MAP_PATHS[eval_type]at all (just use_resolved_evaluator_map) - Setting it to a sentinel string that provides a better error message if accidentally resolved
Additional Comments (1)
|
1 participant
Adds a lightweight
nemo-skills-coresubpackage (core/subdirectory)with only inference, evaluation, and tool calling deps. Default
pip install nemo-skillsis unchanged (installs everything).Changes
core/pyproject.toml+core/requirements.txt: New subpackage installable viapip install ./coreor git URL with#subdirectory=core. Single source of truth for core deps, referenced by both core and rootpyproject.toml.nemo_skills/pipeline/__init__.py: Import guard usingimportlib.metadata-- importing pipeline modules with only core installed raises a clearImportErrorinstead of a crypticModuleNotFoundError.nemo_skills/_cli_stub.py: StubnsCLI entry point for core-only installs that prints a helpful message.nemo_skills/evaluation/evaluator/__init__.py: Lazy evaluator registry using string paths instead of eager imports, so core-only installs don't fail on benchmark-specific deps (faiss,func_timeout, etc.).nemo_skills/dataset/utils.py+nemo_skills/pipeline/dataset.py: Moved cluster-dependent dataset logic into pipeline module to keep core free ofnemo_runimports.requirements/pipeline.txt: New requirements file for pipeline-only deps (nemo_run,typer, etc.)..github/workflows/tests.yml: Installuvin CI for use with testing installation.Summary by CodeRabbit
New Features
Documentation
Chores