Skip to content

fix: update timeout of RLVR sequence_length test - #6180

Open
rsareddy0329 wants to merge 3 commits into
aws:masterfrom
rsareddy0329:master-canary-test-fixes
Open

fix: update timeout of RLVR sequence_length test#6180
rsareddy0329 wants to merge 3 commits into
aws:masterfrom
rsareddy0329:master-canary-test-fixes

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 Aug 13, 2026

Copy link
Copy Markdown

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


PR Review

  • Read PR diff (/tmp/pr.diff)
  • Review the change for correctness & best practices

The change bumps max_wait_time from 3600 to 7200 seconds in test_rlvr_trainer_lora_with_sequence_length, with a clear explanatory comment noting that sequence_length training takes >1 hour.

Looks good. This is a straightforward, correct fix for a test that was timing out. The polling loop (poll_interval = 30) and terminal-status handling are unchanged and remain correct — the loop will still break early on Completed/Failed/Stopped, so the longer timeout only affects the worst-case wait, not the happy path.

Minor (optional, non-blocking) observations — not required for this PR:

  • If the job never reaches a terminal state, the final assert training_job.training_job_status == "Completed" will now surface after 2 hours rather than 1. That's the intended tradeoff here, so no action needed.
  • Consider whether an integ test blocking up to 2 hours fits within your canary/CI budget; if this becomes a pattern, a shared timeout constant might be cleaner. Not something to address in this PR.

No correctness, security, or backward-compatibility concerns. 👍

@github-actions

github-actions Bot commented Aug 13, 2026

Copy link
Copy Markdown

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


PR Review

  • Read PR diff (/tmp/pr.diff)
  • Review changes for correctness & best practices

This PR makes two small, test-only changes:

  1. test_nova_model_customization_deployment.py — adds @pytest.mark.xdist_group("bedrock_deployment") to TestNovaBedrockDeployment. This pins all tests in the class to the same pytest-xdist worker, serializing Bedrock deployment tests so they don't run concurrently and collide on shared Bedrock resources. Sensible fix for flaky parallel runs.

  2. test_rlvr_trainer_integration.py — bumps max_wait_time from 36007200 in test_rlvr_trainer_lora_with_sequence_length, with a clear comment explaining that sequence_length training takes >1 hour. The polling loop (poll_interval = 30) and terminal-status handling are unchanged, so the longer timeout only affects the worst case — the loop still breaks early on Completed/Failed/Stopped.

Looks good. Both are correct, low-risk test infrastructure fixes with no impact on public SDK behavior. No correctness, security, or backward-compatibility concerns. 👍

Minor (optional, non-blocking):

  • An integ test that can now block up to 2 hours is worth keeping an eye on for canary/CI budget. If timeouts like this become a recurring pattern, a shared timeout constant might be cleaner — not needed for this PR.

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