Repository navigation
Add radioactive decay MCP tool - #1416
tamohannes wants to merge 10 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a FastMCP server and stdio-launched client wrapper providing radioactive-decay tools ( Changes
Sequence DiagramsequenceDiagram
actor User
participant Client
participant MCPStdioClient
participant Server as radioactivedecay_tool
participant Library as radioactivedecay
User->>Client: call decay_chain(nuclide, time, time_unit)
Client->>MCPStdioClient: send tool request (stdio)
MCPStdioClient->>Server: invoke tool handler
Server->>Server: validate inputs (nuclide, time, unit)
Server->>Library: construct Nuclide & Inventory
Library-->>Server: Nuclide / Inventory
Server->>Library: Inventory.decay(time, time_unit)
Library-->>Server: decayed inventory
Server->>Server: compute activities, filter, format table
Server-->>MCPStdioClient: return formatted response
MCPStdioClient-->>Client: deliver result
Client-->>User: present decay chain data
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 unit tests (beta)
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: 1
🧹 Nitpick comments (1)
tests/test_mcp_clients.py (1)
1018-1034: ⚡ Quick winAdd one runtime tool-call smoke assertion in this test.
This test currently validates discovery/schema only. Adding a simple
execute(...)call would catch runtime regressions in the stdio call path with minimal extra cost.Suggested extension (inside the same test)
try: tools = await tool.list_tools() tool_names = {t["name"] for t in tools} assert "nuclide-info" in tool_names assert "decay-chain" in tool_names decay_tool = next(t for t in tools if t["name"] == "decay-chain") schema_props = decay_tool["input_schema"]["properties"] assert "time_unit" not in schema_props + + out = await tool.execute("decay-chain", {"nuclide": "Co-60", "time": 1.0}) + assert isinstance(out, str) + assert "Decay of" in out finally: await tool.shutdown()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_mcp_clients.py` around lines 1018 - 1034, The test only checks discovery/schema; add a runtime smoke invocation to catch stdio regressions by calling RadioactivedecayTool.execute for the discovered "decay-chain" tool and asserting the response is truthy/has expected shape. Locate the test_radioactivedecay_stdio_list_tools test, after you select decay_tool (next(t for t in tools if t["name"] == "decay-chain")), call tool.execute with a minimal valid input payload for the decay-chain tool (e.g., a simple nuclide and time spec) and assert the result is non-empty or contains an expected key (like a decay chain entry), then proceed to the existing finally block to await tool.shutdown().
🤖 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/radioactivedecay_tool.py`:
- Around line 58-67: The loop that formats the half-life (using nuc.half_life()
and nuc.half_life(unit)) only emits a line when a converted value is >= 1.0, so
nuclides with half-life < 1 s produce no output; update the logic in the
half-life block to ensure a value is always appended — after the for loop over
units (["y","d","h","m","s"]) detect if no line was appended and then append a
fallback line (e.g., use the raw seconds value half_life_s or the seconds
conversion hl when <1) such as "Half-life: {half_life_s:.6g} s" so sub-second
half-lives are shown; make the change around the code that references
half_life_s, nuc.half_life(), and the existing for-loop.
---
Nitpick comments:
In `@tests/test_mcp_clients.py`:
- Around line 1018-1034: The test only checks discovery/schema; add a runtime
smoke invocation to catch stdio regressions by calling
RadioactivedecayTool.execute for the discovered "decay-chain" tool and asserting
the response is truthy/has expected shape. Locate the
test_radioactivedecay_stdio_list_tools test, after you select decay_tool (next(t
for t in tools if t["name"] == "decay-chain")), call tool.execute with a minimal
valid input payload for the decay-chain tool (e.g., a simple nuclide and time
spec) and assert the result is non-empty or contains an expected key (like a
decay chain entry), then proceed to the existing finally block to await
tool.shutdown().
🪄 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: 58d9a4f0-29ef-4ce3-bb2e-f64bd6c27d22
📒 Files selected for processing (3)
docs/agentic_inference/tool_calling.mdnemo_skills/mcp/servers/radioactivedecay_tool.pytests/test_mcp_clients.py
gwarmstrong
left a comment
gwarmstrong
left a comment
There was a problem hiding this comment.
Hi @tamohannes , Thank you for the contributions! I would encourage you to please switch to a tool path that avoids the MCP protocol (we have found it to cause real latency and stability issues during RL) in favor of a direct tool approach. The changes should be fairly minimal--please see the DirectPythonTool for an example.
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 @.github/workflows/tests.yml:
- Line 27: The step-level conditional using "if: ${{ secrets.GHCR_PAT != '' }}"
is invalid; move the secret into the job-level environment and change the
conditional to check env instead. Define GHCR_PAT under the job's env (e.g.,
env: GHCR_PAT: ${{ secrets.GHCR_PAT }}) and update the step conditional to "if:
${{ env.GHCR_PAT != '' }}", ensuring the step no longer references the secrets
context directly.
🪄 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: 866ec0e0-0f22-41e2-a641-10facb91e30c
📒 Files selected for processing (1)
.github/workflows/tests.yml
Signed-off-by: tamohannes <hovhannes.tamoyan@gmail.com>
6aab2a9 to
38238f6
Compare
Signed-off-by: tamohannes <hovhannes.tamoyan@gmail.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/tests.yml (1)
26-32:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix the GHCR login gate before merging.
secretsis still referenced in the step-levelif, which GitHub Actions rejects during workflow validation. The build step’senvblock does not help here because step conditions cannot read it.Suggested fix
jobs: unit-tests: runs-on: ubuntu-latest + env: + GHCR_PAT: ${{ secrets.GHCR_PAT }} steps: @@ - name: Login to GHCR - if: ${{ secrets.GHCR_PAT != '' }} + if: ${{ env.GHCR_PAT != '' }} uses: docker/login-action@v3 with: registry: ghcr.io username: ${{ github.actor }} - password: ${{ secrets.GHCR_PAT }} + password: ${{ env.GHCR_PAT }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests.yml around lines 26 - 32, The step "Login to GHCR" is using a step-level if that references secrets.GHCR_PAT (secrets cannot be evaluated in step-level conditionals and fails workflow validation); remove the step-level if that uses secrets.GHCR_PAT and instead place the conditional at the job level (or gate the entire job) so the login step using docker/login-action@v3 only runs when the secret exists; ensure the step itself no longer references secrets in its if and continues to use username: ${{ github.actor }} and password: ${{ secrets.GHCR_PAT }} inside the with: block.
🤖 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/radioactivedecay_tool.py`:
- Around line 96-97: The current validation only checks time < 0 and allows
NaN/inf to reach inv.decay; update the validation around the variable time (the
same block that currently does "if time < 0: return ...") to first reject
non-finite values (use math.isfinite or equivalent) and return a clear error
(e.g., "Time must be a finite non-negative number") before proceeding to call
inv.decay(...), so inv.decay never receives NaN or infinite inputs.
---
Duplicate comments:
In @.github/workflows/tests.yml:
- Around line 26-32: The step "Login to GHCR" is using a step-level if that
references secrets.GHCR_PAT (secrets cannot be evaluated in step-level
conditionals and fails workflow validation); remove the step-level if that uses
secrets.GHCR_PAT and instead place the conditional at the job level (or gate the
entire job) so the login step using docker/login-action@v3 only runs when the
secret exists; ensure the step itself no longer references secrets in its if and
continues to use username: ${{ github.actor }} and password: ${{
secrets.GHCR_PAT }} inside the with: block.
🪄 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: 2fd5aea4-a2a9-4049-a681-1c4e9bc4f108
📒 Files selected for processing (4)
.github/workflows/tests.ymldocs/agentic_inference/tool_calling.mdnemo_skills/mcp/servers/radioactivedecay_tool.pytests/test_mcp_clients.py
✅ Files skipped from review due to trivial changes (1)
- docs/agentic_inference/tool_calling.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_mcp_clients.py
Signed-off-by: tamohannes <hovhannes.tamoyan@gmail.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Signed-off-by: tamohannes <hovhannes.tamoyan@gmail.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Signed-off-by: tamohannes <hovhannes.tamoyan@gmail.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Signed-off-by: tamohannes <hovhannes.tamoyan@gmail.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Signed-off-by: tamohannes <hovhannes.tamoyan@gmail.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Signed-off-by: tamohannes <hovhannes.tamoyan@gmail.com>
|
Updated this PR to use the direct \ path instead of launching a stdio MCP server. The PR diff is now limited to the tool implementation, docs reference, and focused tests; workflow/pyproject changes were removed. @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
|
Updated this PR to use the direct Tool path instead of launching a stdio MCP server. The PR diff is now limited to the tool implementation, docs reference, and focused tests; workflow/pyproject changes were removed. @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
Signed-off-by: tamohannes <hovhannes.tamoyan@gmail.com>
|
Updated to the direct Tool implementation path (no stdio MCP server/client). The final diff is scoped to the tool file, docs reference, and focused tests only. @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
…cay-mcp-tool Signed-off-by: tamohannes <hovhannes.tamoyan@gmail.com>
|
Closing in favor of #1423, reopened from an internal NVIDIA-NeMo/Skills branch so CI can access repository secrets. |
Summary
Test plan
Made with Cursor
Summary by CodeRabbit
New Features
Documentation
Tests
Chores