Fix nsys subfield merging behavior
#795
Conversation
juntaowww
requested review from
alexmanle,
amaslenn,
jeffnvidia and
srivatsankrishnan
as code owners
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR changes nsys serialization to exclude unset fields during TestRunModel.tdef_model_dump and adds a new TestNsysMerging test suite (four tests) validating merging behavior of NsysConfiguration between base NCCLTestDefinition and scenario overrides. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
Greptile OverviewGreptile SummaryFixed Key Changes:
Impact: Confidence Score: 5/5
Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
🤖 Fix all issues with AI agents
In `@tests/test_test_scenario.py`:
- Around line 520-653: Remove the behavioral docstrings inside the
TestNsysMerging test methods: delete the triple-quoted strings at the top of
test_nsys_partial_override_preserves_base_config,
test_nsys_multiple_fields_override, test_nsys_scenario_adds_to_base_without_nsys
(and any similar method docstrings such as test_nsys_disable_override) so the
tests only contain assertions and setup; leave any high-level
interface/class-level documentation if needed but strip per-test behavior
descriptions that duplicate the assertions.
| assert tdef.nsys.enable is True | ||
|
|
||
| def test_nsys_multiple_fields_override(self, test_scenario_parser: TestScenarioParser, slurm_system: SlurmSystem): | ||
| from cloudai.core import NsysConfiguration |
There was a problem hiding this comment.
Can it be imported on top of this file? (same for all tests)
There was a problem hiding this comment.
@juntaowww I assume you used an AI assistant to help you with the code. they really tend to bring imports deep inside. could you please share what is the assistant so we could prepare some common agents config (if there's such an option) to let the agent know about the code style?
| "cmd_args": self.cmd_args.model_dump(by_alias=by_alias) if self.cmd_args else None, | ||
| "git_repos": [repo.model_dump() for repo in self.git_repos] if self.git_repos else None, | ||
| "nsys": self.nsys.model_dump() if self.nsys else None, | ||
| "nsys": self.nsys.model_dump(exclude_unset=True) if self.nsys else None, |
There was a problem hiding this comment.
Can't that lead to a dict without required fields set?
BTW, I think this is similar to what you did for reports. Maybe we should have a function merge_models(tdef, tscenario) or something like that? Do you think that will be useful?
Our current approach is "all or nothing", meaning that everything should be either set on one level or another. If we start doing smarter merge, I'd prefer we do it consistently. Overall this feels as a better and friendlier approach.
cc @podkidyshev
There was a problem hiding this comment.
still learning, so my opinion should be rather optional here :)
-
the change seems safe and positive. agree with Andrey that it would be nice to make it consistent across all model merges but I would assume it's on the cloudai dev team's shoulders
-
Can't that lead to a dict without required fields set?
seems safe, if this is the risk you meant
>>> import pydantic
>>> from typing import Optional
>>> class A(pydantic.BaseModel):
... model_config = pydantic.ConfigDict(extra='forbid')
... a: str
... b: Optional[str] = 'b'
...
>>> A.model_validate({'b': '33'})
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Users/ipodkidyshev/projects/cloudai/.venv/lib/python3.10/site-packages/pydantic/main.py", line 705, in model_validate
return cls.__pydantic_validator__.validate_python(
pydantic_core._pydantic_core.ValidationError: 1 validation error for A
a
Field required [type=missing, input_value={'b': '33'}, input_type=dict]
For further information visit https://errors.pydantic.dev/2.11/v/missing3 participants
Summary
When specifying
[Tests.nsys]in a scenario file to override only specificnsysconfiguration fields (like output), the entirensysobject was being overwritten instead of just the specified fields.Fix:
to:
Using
exclude_unset=Trueensures that only fields explicitly set by the user in the scenario file are included in the dump. Thedeep_mergefunction then correctly merges only those specified fields with the test configuration, preserving all othernsyssettings from the test definition.Test Plan
TestNsysMergingis added totests/test_test_scenario.pyfor testing the correctness of overwriting ofnsysfields from scenario configuration.