-
Notifications
You must be signed in to change notification settings - Fork 193
Add nemo-skills-core subpackage for lightweight installs #1229
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 5 commits
f0eb8d0
9adc401
0a4f056
e2361e6
cc70501
a0a2aa7
7cf5fb6
9e84d39
ae191da
8118b5a
fdcfdb1
49aed5e
d1c4195
b7e258f
c5d12cf
3b025ec
836160f
cf4b94c
6255b1a
b76b8bd
49f3cde
305cde7
28de048
ded4a2c
d173121
d91add1
97bac87
cc4118c
1c0eb66
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 |
|---|---|---|
|
|
@@ -49,6 +49,49 @@ The following things are required when adding new benchmarks | |
| the dataset into slurm tests. This is the most comprehensive test we can do by running full | ||
| evaluation on cluster with arbitrary model and check that results are as expected. | ||
|
|
||
| ### Respect the Core / Pipeline dependency boundary | ||
|
Kipok marked this conversation as resolved.
|
||
|
|
||
| NeMo Skills is split into **Core** (agent runtime) and **Pipeline** (orchestration). The rule is simple: | ||
|
|
||
| ``` | ||
| Pipeline can import from Core. | ||
| Core CANNOT import from Pipeline. | ||
| ``` | ||
|
|
||
| Core modules are everything under `nemo_skills/` **except** `nemo_skills/pipeline/`. They must never have top-level imports from `nemo_skills.pipeline` or `nemo_run`. This boundary is enforced by `tests/test_dependency_isolation.py` which verifies that core modules import successfully without pipeline dependencies installed. | ||
|
|
||
| **When adding a new dependency**, put it in the right requirements file: | ||
|
|
||
| | If the dependency is needed for... | Add it to | | ||
| |---|---| | ||
| | Inference, evaluation, math/code grading, MCP clients, prompt formatting | `core/requirements.txt` | | ||
| | CLI commands, cluster orchestration, experiment tracking | `requirements/pipeline.txt` | | ||
| | Everything else (dataset-specific deps, benchmark-specific packages) | `requirements/main.txt` only | | ||
|
|
||
| Dependencies in `core/requirements.txt` should be things that a typical `GenerationTask` run with PythonTool would need. Dataset-specific or benchmark-specific packages (e.g., `faiss-cpu`, `sacrebleu`, `func-timeout`) go only in `requirements/main.txt`. | ||
|
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 part I don't fully understand - I think benchmark-specific packages should go to core for now as otherwise the code will fail when those benchmarks are used e.g. in evaluator. Eventually we should migrate to jit install, but it's not done yet, so I'd put those into core
Collaborator
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. Fair. My original scope was pretty PythonTool specific, but I think we can come up with something that makes a little more sense in terms of aligning the core code with core dependencies.
Collaborator
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. yeah it's now in core and there is a clearer description of what's in |
||
|
|
||
| All core and pipeline deps must also appear in `requirements/main.txt` (the monolithic file used for default installs). | ||
|
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. can we not link multiple requirements listed in pyproject.toml? We duplicate?
Collaborator
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. we should be able to do that. It should be implemented that way now--I updated this at one point so it links against the file rather than duplicating. Will fix.
Collaborator
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 |
||
|
|
||
| **Examples of correct placement:** | ||
|
|
||
| - `httpx` -> `core/requirements.txt` (used by model inference clients) | ||
| - `sympy` -> `core/requirements.txt` (used by math graders) | ||
|
Kipok marked this conversation as resolved.
Outdated
|
||
| - `nemo_run` -> `requirements/pipeline.txt` (cluster job orchestration) | ||
| - `wandb` -> `requirements/pipeline.txt` (experiment tracking for cluster jobs) | ||
| - `faiss-cpu` -> `requirements/main.txt` only (only needed for BFCL benchmark) | ||
|
|
||
| **Examples of mistakes to avoid:** | ||
|
|
||
| - Adding `nemo_run` to `core/requirements.txt` -- it is a pipeline/orchestration dependency, core must not depend on it. | ||
| - Adding `typer` to `core/requirements.txt` -- it is the CLI framework, only used by the pipeline layer. | ||
| - Adding a new dependency only to a subpackage file but forgetting `requirements/main.txt` -- the default install would be missing it. | ||
|
|
||
| **When writing new core code:** | ||
|
|
||
| - If you need something from `nemo_skills.pipeline`, your code probably belongs in pipeline, not core. Move it. | ||
| - If you have a function that works locally but *also* needs a cluster variant, put the local version in core and a cluster-aware wrapper in `nemo_skills/pipeline/` (see `pipeline/dataset.py` for the pattern). | ||
|
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. I actually think that if we have a case like this, it means we need to redesign something. Ideally separation should be clean, and we shouldn't need to duplicate functionality. E.g. the dataset module part is a bit messy and there is probably a way to do it better, such that there is a pipeline level that only manages pulling from cluster and then there is a local level that always assumes things are present locally and is being called inside pipeline directly
Collaborator
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. Makes sense, updated the docs here to reflect that and made the implementation more consistent with the guidance here/there. |
||
| - If you absolutely must reference pipeline code from core for backwards compatibility, use a lazy import inside a function body with a `DeprecationWarning` (see `dataset/utils.py:get_dataset_module` for the pattern). Never add a top-level import. | ||
|
|
||
| ### Keep the code elegant | ||
| When adding new features, try to keep the code simple and elegant. | ||
| - Can you reuse / extend an existing functionality? | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| # Copyright (c) 2025, NVIDIA CORPORATION. All rights reserved. | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # 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. | ||
|
|
||
| [build-system] | ||
| requires = [ | ||
| "setuptools", | ||
| "wheel" | ||
| ] | ||
| build-backend = "setuptools.build_meta" | ||
|
|
||
| [project] | ||
| dynamic = ["version", "dependencies"] | ||
|
|
||
| name = "nemo-skills-core" | ||
| description = "NeMo Skills core runtime -- inference, evaluation, and tool calling" | ||
| readme = {text = "NeMo Skills core runtime for inference, evaluation, and tool calling. See https://nvidia-nemo.github.io/Skills for full documentation.", content-type = "text/plain"} | ||
| classifiers = [ | ||
| "Programming Language :: Python :: 3", | ||
| "Programming Language :: Python :: 3.10", | ||
| "License :: OSI Approved :: Apache Software License", | ||
| "Operating System :: OS Independent", | ||
| ] | ||
| requires-python = ">=3.10" | ||
|
|
||
| [project.urls] | ||
| homepage = "https://nvidia-nemo.github.io/Skills" | ||
| source = "https://github.com/NVIDIA-NeMo/Skills" | ||
| issues = "https://github.com/NVIDIA-NeMo/Skills/issues" | ||
|
|
||
| [project.scripts] | ||
| ns = "nemo_skills._cli_stub:main" | ||
|
|
||
| [tool.setuptools] | ||
| include-package-data = true | ||
|
|
||
| [tool.setuptools.packages.find] | ||
| where = [".."] | ||
| exclude = ["tests", "tests.*", "core", "core.*"] | ||
|
|
||
| [tool.setuptools.dynamic] | ||
| version = { attr = "nemo_skills.version.__version__" } | ||
| dependencies = {file = ["requirements.txt"]} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| # Core dependencies for agent runtime (inference, evaluation, tool calling, math/code grading). | ||
| # No cluster orchestration deps (nemo_run, typer, etc.) | ||
|
|
||
|
|
||
| # --- code evaluation --- | ||
|
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. are you sure this covers all benchmarks? Generally, we should move to keeping this reqs really simple and move most benchmark-specific requirements to install at runtime, but for now probably we might need some more packages here? E.g. datasets is almost certainly needed and then other benchmark specific things, like sacrebleu, etc.
Collaborator
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. I revisited the separation. This should contain all the reqs not needed for cluster orchestration now. |
||
| compute-eval @ git+https://github.com/NVIDIA/compute-eval.git@2d14770 | ||
| evalplus @ git+https://github.com/evalplus/evalplus@c91370f | ||
| fire | ||
| flask | ||
| # --- agent runtime --- | ||
| httpx | ||
| huggingface_hub | ||
| hydra-core | ||
| ipython | ||
| litellm[caching] | ||
|
|
||
| # --- math evaluation --- | ||
| math-verify[antlr4_9_3] | ||
| mcp | ||
| numpy | ||
| openai | ||
| pyyaml | ||
| requests | ||
| rich | ||
| sympy | ||
| tqdm | ||
| transformers | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| # Installation & Dependency Groups | ||
|
|
||
| NeMo Skills provides two installable packages: | ||
|
|
||
| - **`nemo-skills`** (root) -- full install with CLI, cluster orchestration, all benchmarks | ||
| - **`nemo-skills-core`** (`core/` subdirectory) -- lightweight runtime only | ||
|
|
||
| ## Default installation | ||
|
|
||
| `pip install nemo-skills` gives you **everything** (inference, evaluation, CLI, | ||
| cluster orchestration, benchmarks): | ||
|
|
||
| ```bash | ||
| pip install git+https://github.com/NVIDIA-NeMo/Skills.git | ||
| # or, from a local clone: | ||
| pip install -e . | ||
| ``` | ||
|
|
||
| ## Lightweight installation | ||
|
|
||
| If you only need inference, evaluation, and tool calling (no cluster orchestration): | ||
|
|
||
| ```bash | ||
| pip install "nemo-skills-core @ git+https://github.com/NVIDIA-NeMo/Skills.git#subdirectory=core" | ||
| # or, from a local clone: | ||
| pip install -e core/ | ||
| ``` | ||
|
|
||
| ## Extras (dependency groups) | ||
|
|
||
| | Extra | Requirements file | What it provides | | ||
| |-------|-------------------|------------------| | ||
| | `core` | `core/requirements.txt` | Agent runtime: inference, evaluation, tool calling (MCP), prompt formatting, math/code grading. No cluster orchestration. | | ||
| | `pipeline` | `requirements/pipeline.txt` | CLI (`ns` command), cluster management, experiment tracking (`nemo_run`, `typer`, `wandb`). | | ||
| | `dev` | `requirements/common-tests.txt`, `requirements/common-dev.txt` | Development and testing tools (`pytest`, `ruff`, `pre-commit`). | | ||
|
|
||
| ### Examples | ||
|
|
||
| ```bash | ||
| # Full install (default) | ||
| pip install -e . | ||
|
|
||
| # Core only -- lightweight runtime for downstream integrations | ||
| pip install -e core/ | ||
|
|
||
| # Development (everything + dev tools) | ||
| pip install -e ".[dev]" | ||
| ``` | ||
|
|
||
| ## Core / Pipeline architecture boundary | ||
|
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. I'd maybe move this part somewhere else (e.g. only keep in contributing.md or better in a new .md where extra details from contributing can go as well). It's helpful, but probably a bit too dense for the "basics" part of the docs. More oriented towards people who'd need to modify our code
Collaborator
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. okay, put it in the |
||
|
|
||
| The codebase enforces a one-way dependency rule: pipeline modules can import | ||
| from core, but core modules must not import from pipeline. | ||
|
|
||
| This boundary is enforced by `tests/test_dependency_isolation.py` which creates | ||
|
Kipok marked this conversation as resolved.
Outdated
|
||
| fresh virtualenvs and verifies that core modules import successfully without | ||
|
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. is this true? I don't think we create fresh envs in tests?
Collaborator
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. yeah this is outdated from old tests, good cattch |
||
| pipeline dependencies installed. | ||
|
|
||
| ### Dataset loading example | ||
|
|
||
| The boundary shows up concretely in dataset loading: | ||
|
|
||
| ```python | ||
| # Core: local-only dataset loading (no cluster deps) | ||
| from nemo_skills.dataset.utils import get_dataset_module | ||
| module, path, on_cluster = get_dataset_module("gsm8k") | ||
|
|
||
| # Pipeline: cluster-aware dataset loading (SSH tunnels, mount resolution) | ||
| from nemo_skills.pipeline.dataset import get_dataset_module | ||
| module, path, on_cluster = get_dataset_module("gsm8k", cluster_config=cfg) | ||
| ``` | ||
|
|
||
| If you pass `cluster_config` to the core version, it will emit a | ||
| `DeprecationWarning` and redirect to the pipeline version. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| # Copyright (c) 2025, NVIDIA CORPORATION. All rights reserved. | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # 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 sys | ||
|
|
||
|
|
||
| def main(): | ||
| print("nemo-skills-core is installed (lightweight mode).\nFor the full ns CLI, run: pip install nemo-skills") | ||
| sys.exit(1) |
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.
do we use it anywhere?
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.
good catch, it was from an old testing strategy, removed