Skip to content

fix infer logic of agent_run_time from the attached input trainer - #6115

Open
rsareddy0329 wants to merge 5 commits into
aws:masterfrom
rsareddy0329:gpu-mtrl-eval-test-issue
Open

fix infer logic of agent_run_time from the attached input trainer#6115
rsareddy0329 wants to merge 5 commits into
aws:masterfrom
rsareddy0329:gpu-mtrl-eval-test-issue

Conversation

@rsareddy0329

Copy link
Copy Markdown
Contributor

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown

Claude finished @rsareddy0329's task in 55s —— View job


Code Review

  • Read PR diff (/tmp/pr.diff)
  • Review source change in multi_turn_rl_evaluator.py
  • Review added/updated tests

The change is focused and correct: _resolve_trainer_defaults now handles the nested shape that AgentRFTJob.agent_config actually returns ({"BedrockAgentCoreConfig": {"AgentRuntimeArn": ...}} / {"CustomAgentLambdaConfig": {"LambdaArn": ...}}) while preserving the old flat-key fallbacks. Customer-provided agent_config still wins (guarded by if self.agent_config is None), so it's backward compatible. Test coverage is good — both nested shapes, both flat fallbacks, and the "customer value not overwritten" case.

A couple of minor points, neither blocking:

1. Potential AttributeError if a config key is present but None.

bedrock_cfg = resolved_agent.get("BedrockAgentCoreConfig", {})
lambda_cfg = resolved_agent.get("CustomAgentLambdaConfig", {})

dict.get(key, {}) only returns the {} default when the key is absent. If the key is present with an explicit None value (e.g. {"BedrockAgentCoreConfig": None}), bedrock_cfg becomes None and the following bedrock_cfg.get(...) raises AttributeError. If the upstream job can emit that shape, prefer resolved_agent.get("BedrockAgentCoreConfig") or {} to coerce None to {}.

2. Unit test relies on type(trainer).__name__ = "MultiTurnRLTrainer" on a MagicMock.
This works because _resolve_trainer_defaults gates on the class name, and it's a nice way to exercise the parsing logic in isolation — no concern, just noting it's a somewhat brittle pattern if the gating check ever changes to isinstance.

Overall this looks good to merge. The core fix is sound and well-tested.

@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown

Claude finished @rsareddy0329's task in 43s —— View job


Code Review

  • Read PR diff (/tmp/pr.diff)
  • Review source change in multi_turn_rl_evaluator.py
  • Review added/updated tests

The core fix is sound: _resolve_trainer_defaults now unpacks the nested shape AgentRFTJob.agent_config actually returns ({"BedrockAgentCoreConfig": {"AgentRuntimeArn": ...}} / {"CustomAgentLambdaConfig": {"LambdaArn": ...}}) while keeping the flat-key fallbacks, and the if self.agent_config is None guard preserves customer-provided values. Coverage is good — both nested shapes, both flat fallbacks, and the "don't overwrite customer value" case.

One point from the prior review is still unaddressed in the current diff:

1. AttributeError when a config key is present but None (multi_turn_rl_evaluator.py:260-261).

bedrock_cfg = resolved_agent.get("BedrockAgentCoreConfig", {})
lambda_cfg = resolved_agent.get("CustomAgentLambdaConfig", {})

dict.get(key, {}) returns the {} default only when the key is absent. If the upstream job emits the key with an explicit None value — e.g. {"BedrockAgentCoreConfig": None, "CustomAgentLambdaConfig": {"LambdaArn": "..."}}, which is a very common serialization shape when one of two mutually-exclusive configs is unset — then bedrock_cfg is None and bedrock_cfg.get("AgentRuntimeArn") raises AttributeError. Recommend coercing to {}:

bedrock_cfg = resolved_agent.get("BedrockAgentCoreConfig") or {}
lambda_cfg = resolved_agent.get("CustomAgentLambdaConfig") or {}

This is cheap insurance and the more likely-to-occur shape given only one config is populated at a time. Whether it's blocking depends on whether the producer can emit None-valued keys — worth confirming, and adding a {"BedrockAgentCoreConfig": None, ...} unit case to lock it in.

2. (non-blocking) Unit test uses type(trainer).__name__ = "MultiTurnRLTrainer" on a MagicMock.
Fine as-is since the gating is a class-name string check, but brittle if that gate ever moves to isinstance. Just noting.

Otherwise this looks good — focused, backward compatible, and well tested.

Note: the inline-comment tool wasn't available in this run, so findings are consolidated here with file/line references.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants