Repository navigation
Add periodictable MCP tool - #1421
Merged
Merged
Conversation
Signed-off-by: tamohannes <hovhannes.tamoyan@gmail.com>
Signed-off-by: tamohannes <hovhannes.tamoyan@gmail.com>
Signed-off-by: tamohannes <hovhannes.tamoyan@gmail.com>
Signed-off-by: tamohannes <hovhannes.tamoyan@gmail.com>
Signed-off-by: tamohannes <hovhannes.tamoyan@gmail.com>
Signed-off-by: tamohannes <hovhannes.tamoyan@gmail.com>
Signed-off-by: tamohannes <hovhannes.tamoyan@gmail.com>
Signed-off-by: tamohannes <hovhannes.tamoyan@gmail.com>
…-mcp-tool Signed-off-by: tamohannes <hovhannes.tamoyan@gmail.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (3)
📝 WalkthroughWalkthroughAdds a new MCP-compatible tool module ChangesPeriodictableTool Feature
Sequence DiagramsequenceDiagram
participant Client
participant PeriodictableTool
participant PeriodictableLib
Client->>PeriodictableTool: call execute(tool_name, arguments)
PeriodictableTool->>PeriodictableTool: normalize & dispatch
PeriodictableTool->>PeriodictableLib: resolve element / fetch isotope data
PeriodictableLib-->>PeriodictableTool: element/isotope data
PeriodictableTool-->>Client: formatted string result or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/agentic_inference/tool_calling.md`:
- Around line 374-375: Remove the duplicate bullet for PeriodictableTool: keep a
single entry for nemo_skills.mcp.servers.periodictable_tool.PeriodictableTool
and delete the repeated line so the built-in tools list contains only one
description (choose the clearer wording: "Element, isotope, and neutron
scattering lookup via periodictable (requires `periodictable`)"). Ensure the
remaining bullet uses the correct fully-qualified symbol PeriodictableTool and
its GitHub link.
In `@nemo_skills/mcp/servers/periodictable_tool.py`:
- Around line 66-76: The AttributeError except blocks in periodictable_tool.py
(around the blocks that inspect el.neutron and the later block at lines
~107-118) silently swallow unexpected structure issues; replace the try/except
with explicit attribute probes (e.g., check hasattr(el, "neutron") and then use
getattr(el.neutron, "b_c", None) / getattr(..., "coherent", None) etc.) and only
append lines when those attributes are present and not None; apply the same
pattern to the second neutron-reading block so unexpected AttributeError is not
hidden and real bugs fail fast.
- Line 45: The symbol comparison is case-sensitive (s == el.symbol) causing
inputs like "fe" to miss; update the matching logic in the lookup (the
conditional using s, el.name, el.symbol, el.number) to compare symbol
case-insensitively by normalizing both sides (e.g., compare s.lower() to
el.symbol.lower()), keeping the existing checks for name and number intact so
name/number matching still works.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 888cc25f-4fb1-40db-9656-dad7e7da0442
📒 Files selected for processing (3)
docs/agentic_inference/tool_calling.mdnemo_skills/mcp/servers/periodictable_tool.pytests/test_mcp_clients.py
Signed-off-by: tamohannes <hovhannes.tamoyan@gmail.com>
tamohannes
force-pushed
the
feat/periodictable-mcp-tool
branch
from
May 2, 2026 00:14
1b7857d to
038f763
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nemo_skills/mcp/servers/periodictable_tool.py`:
- Around line 127-130: The configure method currently accepts any keys and
stores them in self._config even though list_tools() and execute() don't use
them; change configure (in class PeriodicTableTool) to validate overrides
against an explicit allowed set (e.g., allowed keys like "some_known_key1",
"some_known_key2" or the actual config fields used by list_tools/execute) and
raise ValueError for any unknown keys so typos/misconfigurations fail fast, or
alternatively implement real consumption by mapping allowed keys into the fields
used by list_tools() and execute(); update the method that references
self._config (list_tools and execute) to read only those validated keys.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 09387e7b-3855-4cd7-9e5c-50c63b7b0ab0
📒 Files selected for processing (2)
docs/agentic_inference/tool_calling.mdnemo_skills/mcp/servers/periodictable_tool.py
✅ Files skipped from review due to trivial changes (1)
- docs/agentic_inference/tool_calling.md
Comment on lines
+127
to
+130
| def configure(self, overrides: dict[str, Any] | None = None, context: dict[str, Any] | None = None) -> None: | ||
| if overrides: | ||
| self._config.update(overrides) | ||
|
|
Contributor
There was a problem hiding this comment.
Reject unsupported configure() overrides instead of silently storing dead config
Line 128-Line 130 currently accepts any override key, but those values never affect list_tools() or execute(). This can hide typos/misconfiguration. Fail fast when overrides are provided (or implement real config consumption).
Suggested fix
class PeriodictableTool(Tool):
def __init__(self) -> None:
- self._config: dict[str, Any] = {}
+ self._config: dict[str, Any] = {}
def default_config(self) -> dict[str, Any]:
- return dict(self._config)
+ return {}
def configure(self, overrides: dict[str, Any] | None = None, context: dict[str, Any] | None = None) -> None:
- if overrides:
- self._config.update(overrides)
+ if overrides:
+ keys = ", ".join(sorted(overrides))
+ raise ValueError(f"Unsupported config for PeriodictableTool: {keys}")As per coding guidelines, "Avoid cases where user-passed parameters are unused; code should fail if user specifies an unsupported argument or if a required argument is missing. Use dataclass or **kwargs syntax to handle this automatically".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nemo_skills/mcp/servers/periodictable_tool.py` around lines 127 - 130, The
configure method currently accepts any keys and stores them in self._config even
though list_tools() and execute() don't use them; change configure (in class
PeriodicTableTool) to validate overrides against an explicit allowed set (e.g.,
allowed keys like "some_known_key1", "some_known_key2" or the actual config
fields used by list_tools/execute) and raise ValueError for any unknown keys so
typos/misconfigurations fail fast, or alternatively implement real consumption
by mapping allowed keys into the fields used by list_tools() and execute();
update the method that references self._config (list_tools and execute) to read
only those validated keys.
Signed-off-by: tamohannes <hovhannes.tamoyan@gmail.com>
cmunley1
added a commit
to NVIDIA-NeMo/Gym
that referenced
this pull request
Jun 16, 2026
## Summary - Add a NeMo-Gym `ns_tools` config that exposes `PeriodictableTool` through the existing NeMo-Skills ToolManager bridge. - Add a focused config test to keep the resource-server wiring valid. - This is the Gym-side companion to NVIDIA-NeMo/Skills#1421. Replaces fork-origin PR #1219 so CI can run with full repository secrets. ## Test plan - `PYTHONPATH=. python -m pytest resources_servers/ns_tools/tests/test_periodictable_mcp_config.py -q --tb=short` --------- Signed-off-by: tamohannes <hovhannes.tamoyan@gmail.com> Co-authored-by: Christian Munley <cmunley@nvidia.com>
ritaneves
pushed a commit
to NVIDIA-NeMo/Gym
that referenced
this pull request
Jun 25, 2026
## Summary - Add a NeMo-Gym `ns_tools` config that exposes `PeriodictableTool` through the existing NeMo-Skills ToolManager bridge. - Add a focused config test to keep the resource-server wiring valid. - This is the Gym-side companion to NVIDIA-NeMo/Skills#1421. Replaces fork-origin PR #1219 so CI can run with full repository secrets. ## Test plan - `PYTHONPATH=. python -m pytest resources_servers/ns_tools/tests/test_periodictable_mcp_config.py -q --tb=short` --------- Signed-off-by: tamohannes <hovhannes.tamoyan@gmail.com> Co-authored-by: Christian Munley <cmunley@nvidia.com> Signed-off-by: Rita Fernandes Neves <rfernandesne@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
NVIDIA-NeMo/Skillsbranch so CI can access repository secrets.Test plan
python -m py_compile nemo_skills/mcp/servers/periodictable_tool.pypython -m pytest tests/test_mcp_clients.py::TestPeriodictableTool -q --tb=shortpython -m pytest tests/test_mcp_clients.py -q --tb=shortSummary by CodeRabbit
New Features
Documentation
Tests