Skip to content

Conversation

@JashG
Copy link
Contributor

@JashG JashG commented Aug 13, 2025

Description

The RailsConfig class allows for adding two config objects. This MR addresses two edge cases where this function throws an error.

I ran into this error upstream using the NeMo Guardrails service. Internally, the /v1/chat/completions endpoint invokes RailsConfig.__add__ when given multiple config IDs. I ran into these two errors when attempting to use this endpoint with multiple sample configs.

Related Issue(s)

Checklist

  • I've read the CONTRIBUTING guidelines.
  • I've updated the documentation if applicable.
  • I've added tests if applicable.
  • [] @mentions of the person or team responsible for reviewing proposed changes.

@codecov-commenter
Copy link

codecov-commenter commented Aug 13, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.61%. Comparing base (6ba7832) to head (dfb4484).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1328      +/-   ##
===========================================
+ Coverage    70.58%   70.61%   +0.02%     
===========================================
  Files          161      161              
  Lines        16291    16303      +12     
===========================================
+ Hits         11499    11512      +13     
+ Misses        4792     4791       -1     
Flag Coverage Δ
python 70.61% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
nemoguardrails/rails/llm/config.py 90.53% <100.00%> (+0.19%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Pouyanpi Pouyanpi added the bug Something isn't working label Aug 14, 2025
@Pouyanpi Pouyanpi added this to the v0.16.0 milestone Aug 14, 2025
@Pouyanpi Pouyanpi requested review from Pouyanpi and Copilot August 14, 2025 13:34
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR addresses edge cases in the RailsConfig.__add__ method where adding two config objects with None values for prompts or config_path would cause errors. The fix ensures proper handling of None values when merging configs and includes comprehensive test coverage for the addition functionality.

  • Adds null-safety checks for prompts field in validation methods
  • Handles None values for config_path when joining configuration paths
  • Introduces comprehensive test suite for RailsConfig addition functionality

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
nemoguardrails/rails/llm/config.py Adds null-safety checks for prompts field and config_path handling
tests/rails/llm/test_config.py Adds comprehensive test coverage for RailsConfig addition scenarios

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@Pouyanpi
Copy link
Collaborator

Thank you @JashG for fixing this bug 👍🏻 can you gpg sign your commit following contributing guidelines?

@JashG JashG force-pushed the fix/railsconfig-addition-none-handling branch from 663f395 to dfb4484 Compare August 14, 2025 14:25
@JashG
Copy link
Contributor Author

JashG commented Aug 14, 2025

@Pouyanpi Done!

Copy link
Collaborator

@Pouyanpi Pouyanpi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you @JashG 👍🏻

@Pouyanpi Pouyanpi merged commit 6430085 into NVIDIA-NeMo:develop Aug 14, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: RailsConfig throws error when adding configs

3 participants